Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix 10279: Set updater endpoint at build time #5854

Merged
merged 2 commits into from
Jun 23, 2020
Merged

Fix 10279: Set updater endpoint at build time #5854

merged 2 commits into from
Jun 23, 2020

Conversation

jumde
Copy link
Contributor

@jumde jumde commented Jun 15, 2020 •
edited
Loading

Resolves brave/brave-browser#10279

Submitter Checklist:

Test Plan:

  1. Set the updater config.
  2. Setup Little Snitch or Fiddler to intercept requests
  3. Open Brave Browser with a clean profile
  4. Navigate to brave://components, verify the following components download successfully:
    • Brave Local Data Update
    • Brave Ad Block Updater
    • Brave Tor Client Updater (Mac/Win/Linux)
    • NTP Sponsored Images (US)
    • Brave SpeedReader Updater
    • Brave HTTPS Everywhere Updater
    • CRLSets - Updates are not functioning correctly - likely fixed by ReadyCallback should be a repeating callback #5847
  5. Repeat the above steps with a clean profile and --use-dev-goupdater-url flag.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@jumde jumde self-assigned this Jun 15, 2020
@jumde jumde force-pushed the updater_endpoint branch 7 times, most recently from 42e0db2 to 1979203 Compare June 17, 2020 15:05
bridiver
bridiver reviewed Jun 17, 2020
View reviewed changes
common/BUILD.gn Outdated
@@ -1,6 +1,18 @@
import("//build/util/branding.gni")
import("//extensions/buildflags/buildflags.gni")

declare_args() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go in components/component_updater/BUILD.gn. Components https://github.com/brave/brave-core/pull/5854/files#diff-5fd6956fd190ef97f5703d18f6c1fa5d cannot depend on //brave/common

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver - Moved to components/brave_component_updater/browser - let me know if that's good - https://github.com/brave/brave-core/pull/5854/files#diff-deb253279892c75adef317ec6e816ce8

@jumde jumde force-pushed the updater_endpoint branch 4 times, most recently from 18e5080 to 8394318 Compare June 17, 2020 19:05
@jumde jumde requested a review from bridiver June 17, 2020 19:06
bridiver
bridiver reviewed Jun 18, 2020
View reviewed changes
components/greaselion/browser/BUILD.gn Outdated
@@ -11,6 +11,10 @@ source_set("browser") {
"switches.h",
]

configs += [
"//brave/components/brave_component_updater/browser:network_config"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just make this a public config of brave/components/brave_component_updater/browser

bridiver
bridiver reviewed Jun 18, 2020
View reviewed changes
browser/net/BUILD.gn Outdated
configs += [ "//brave/build/geolocation" ]
configs += [
"//brave/build/geolocation",
"//brave/components/brave_component_updater/browser:network_config",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would stay as-is with https://github.com/brave/brave-core/pull/5854/files#r442516329 because this doesn't have or need a dep on //brave/components/brave_component_updater/browser

bridiver
bridiver reviewed Jun 18, 2020
View reviewed changes
components/brave_component_updater/browser/BUILD.gn Outdated
updater_prod_endpoint = ""
}

config("network_config") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - this name doesn't really make sense to me, I could call it either updater_config or just config and make it a public config for browser as mentioned below

@jumde jumde force-pushed the updater_endpoint branch 3 times, most recently from 41e1851 to 92d19f9 Compare June 18, 2020 22:51
bridiver
bridiver reviewed Jun 19, 2020
View reviewed changes
components/brave_component_updater/browser/BUILD.gn Outdated
source_set("browser") {
sources = [
"brave_component.cc",
"brave_component.h",
"brave_component_features.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow established patterns: brave_component_features.cc -> features.cc and the same thing with switches.cc

@jumde jumde force-pushed the updater_endpoint branch 2 times, most recently from dec2352 to 1fe23d8 Compare June 21, 2020 23:30
@jumde jumde force-pushed the updater_endpoint branch 2 times, most recently from 6ee0c25 to db3ab1b Compare June 22, 2020 02:29
iefremov
iefremov reviewed Jun 22, 2020
View reviewed changes
components/brave_component_updater/browser/features.cc Outdated

#include "brave/components/brave_component_updater/browser/features.h"

namespace features {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be brave_component_updater

iefremov
iefremov reviewed Jun 22, 2020
View reviewed changes
components/brave_component_updater/browser/features.h Outdated
#define BRAVE_COMPONENTS_BRAVE_COMPONENT_UPDATER_BROWSER_FEATURES_H_

#include "base/feature_list.h"
#include "build/build_config.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is header is needed, at least it can be moved to .cc ?

iefremov
iefremov reviewed Jun 22, 2020
View reviewed changes
components/brave_component_updater/browser/switches.cc Outdated

#include "brave/components/brave_component_updater/browser/switches.h"

namespace switches {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace brave_component_updater

iefremov
iefremov reviewed Jun 22, 2020
View reviewed changes
components/brave_component_updater/browser/switches.cc Outdated
@@ -0,0 +1,13 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra blank line

iefremov
iefremov reviewed Jun 22, 2020
View reviewed changes
components/brave_component_updater/browser/switches.h Outdated

namespace switches {

extern const char kUseGoUpdateDev[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be constexpr char kUseGoUpdateDev[] = " ..", this way .cc is not needed

@jumde jumde merged commit e7367b8 into master Jun 23, 2020
@jumde jumde deleted the updater_endpoint branch June 23, 2020 00:28
@jumde jumde added this to the 1.12.x - Nightly milestone Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@iefremov iefremov iefremov approved these changes

@mihaiplesa mihaiplesa Awaiting requested review from mihaiplesa

@pilgrim-brave pilgrim-brave Awaiting requested review from pilgrim-brave

Assignees

@jumde jumde

Labels
None yet
Projects
None yet
Milestone
1.12.x - Release #1
Development

Successfully merging this pull request may close these issues.

[Desktop] Set the updater endpoint at build time
3 participants
@jumde @bridiver @iefremov