Brave Vpn api integration #9131

deeppandya · 2021-06-15T08:05:23Z

Resolves brave/brave-browser#14048

Submitter Checklist:

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)
Requested a security/privacy review as needed

Reviewer Checklist:

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:

components/brave_vpn/BUILD.gn

components/brave_vpn/brave_vpn_service.h

simonhong · 2021-06-15T08:33:34Z

browser/sources.gni

@@ -329,6 +330,7 @@ brave_chrome_browser_defines = []

 brave_chrome_browser_sources += brave_browser_autocomplete_sources
 brave_chrome_browser_sources += brave_browser_binance_sources
+brave_chrome_browser_sources += brave_browser_brave_vpn_sources


Curious we will add all new files/deps here instead of creating our new targets even if there is no circular? @iefremov @bridiver
Sorry if both of you discussed about this and shared already :)

As far as I'm aware there is no consensus (at least, with myself :)), but the current approach is having source.gni only for circular dependencies on the Big Chrome

components/brave_vpn/brave_vpn_service.h

components/brave_vpn/brave_vpn_service.cc

components/brave_vpn/brave_vpn_service.h

browser/brave_vpn/brave_vpn_service_factory.cc

browser/brave_vpn/sources.gni

browser/brave_vpn/android/brave_vpn_native_worker.cc

simonhong · 2021-06-15T12:12:10Z

gn_check caught below. We need to create DEPS to brave_vpn components with //services/network/public/cpp.

[2021-06-15T10:08:28.889Z] ERROR in C:\tion\src\brave\components\brave_vpn\brave_vpn_service.cc

[2021-06-15T10:08:28.889Z]   Illegal include: "services/network/public/cpp/shared_url_loader_factory.h"

[2021-06-15T10:08:28.889Z]     Because of no rule applying.

[2021-06-15T10:08:28.889Z]   Illegal include: "services/network/public/cpp/simple_url_loader.h"

[2021-06-15T10:08:28.889Z]     Because of no rule applying.

simonhong · 2021-06-17T00:09:29Z

components/brave_vpn/brave_vpn_service.cc

+          json, base::JSONParserOptions::JSON_PARSE_RFC);
+  base::Optional<base::Value>& records_v = value_with_error.value;
+  const base::Value* subscriber_credential =
+      records_v->FindKey("subscriber-credential");


Oops, I missed one more. Need to check records_v is dictionary.
If |records_v| is not dictionary, calling FindKey on it will give DCHECK failure.

simonhong

++ :)

iefremov · 2021-06-17T08:23:52Z

components/brave_vpn/brave_vpn_service.h

+                           const std::string& product_type);
+
+ private:
+  static GURL oauth_endpoint_;


these two are not used?

iefremov · 2021-06-17T08:24:45Z

components/brave_vpn/brave_vpn_service.cc

+  if (set_app_ident) {
+    request->headers.SetHeader("GRD-App-Ident", "Brave-Client");
+  }
+  auto url_loader = network::SimpleURLLoader::Create(


there is a new utility called BraveApiRequest that probably could be used here to reduce boilerplate

iefremov · 2021-06-17T08:25:21Z

components/brave_vpn/brave_vpn_service.h

+
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  SimpleURLLoaderList url_loaders_;
+  base::WeakPtrFactory<BraveVpnService> weak_factory_;


iefremov · 2021-06-17T08:25:50Z

components/brave_vpn/brave_vpn_service.h

+                           const std::unique_ptr<std::string> response_body);
+
+  void OnGetAllServerRegions(ResponseCallback callback,
+                             const int status,


const is not needed for primitive arguments

iefremov · 2021-06-17T08:29:08Z

components/brave_vpn/DEPS

@@ -0,0 +1,3 @@
+include_rules = [
+  "+services/network/public/cpp",


you need more deps, e.g. components/keyed_service. base and url are not needed

iefremov · 2021-06-17T08:41:15Z

components/brave_vpn/brave_vpn_service.cc

+
+  bool success = status == 200;
+  if (success) {
+    json_response = body;


could be moved to save few clocks

I gather these 4 identical methods could be replaced by a single one?

components/brave_vpn/brave_vpn_service.h

iefremov · 2021-06-17T09:34:28Z

browser/brave_vpn/android/brave_vpn_native_worker.cc

+      brave_vpn_service_(nullptr),
+      weak_factory_(this) {
+  Java_BraveVpnNativeWorker_setNativePtr(env, obj, reinterpret_cast<intptr_t>(this));
+  brave_vpn_service_ = BraveVpnServiceFactory::GetForProfile(


AFAIU this doesn't track the service runtime? the service can die at any moment, so we'd better ensure that we don't use this pointer after this.

I think this wasn't fixed (or I don't understand why the current code is correct). Can you please clarify the lifetime of the worker? I.e. when it is created and when destroyed

So I'd like to understand whether the worker can outlive the service

This concern has been addressed.

iefremov

needs some improvements

iefremov · 2021-06-29T13:19:33Z

components/brave_vpn/brave_vpn_service.h

+#ifndef BRAVE_COMPONENTS_BRAVE_VPN_BROWSER_BRAVE_VPN_SERVICE_H_
+#define BRAVE_COMPONENTS_BRAVE_VPN_BROWSER_BRAVE_VPN_SERVICE_H_
+
+#include <list>


some includes are not used now, please double-check

iefremov · 2021-06-29T13:19:44Z

components/brave_vpn/brave_vpn_service.h

+
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;


not needed?

iefremov · 2021-06-29T13:20:37Z

components/brave_vpn/brave_vpn_service.h

+#include "base/bind.h"
+#include "base/callback_forward.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"


iefremov · 2021-06-29T13:20:55Z

components/brave_vpn/DEPS

+include_rules = [
+  "+services/network/public/cpp",
+  "+base",
+    "+components/keyed_service/core",


iefremov · 2021-06-29T13:50:46Z

browser/brave_vpn/android/brave_vpn_native_worker.h

+
+class BraveVpnService;
+
+namespace chrome {


I think the code convention wants some empty lines in between

iefremov · 2021-06-29T13:56:32Z

everything looks good to me except some codestyle nits and the service/worker lifetime question

Update deps

Address comments on conversation

iefremov · 2021-07-06T08:06:54Z

browser/brave_vpn/android/brave_vpn_native_worker.cc

+void BraveVpnNativeWorker::GetAllServerRegions(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& jcaller) {
+  BraveVpnService* brave_vpn_service = GetBraveVpnService();


nit: you can do if (BraveVpnService* brave_vpn_service = GetBraveVpnService()) {

bsclifton

There are a few oustanding or unresolved comments; those aside, this looks great 😄

I had used this as a reference for understanding a lot of what how VPN works (ex: which steps need to happen in which order; helped a lot for Desktop planning)

deeppandya added this to the 1.27.x - Nightly milestone Jun 15, 2021

deeppandya requested review from bsclifton, simonhong, SergeyZhukovsky and samartnik June 15, 2021 08:05

deeppandya self-assigned this Jun 15, 2021

deeppandya mentioned this pull request Jun 15, 2021

Guardian api integration android #7904

Closed

23 tasks

simonhong reviewed Jun 15, 2021

View reviewed changes

deeppandya requested a review from a team as a code owner June 16, 2021 18:09

deeppandya force-pushed the vpn_api_integration branch from d2b1f08 to b71f1a7 Compare June 16, 2021 18:09

simonhong reviewed Jun 17, 2021

View reviewed changes

simonhong approved these changes Jun 17, 2021

View reviewed changes

deeppandya force-pushed the vpn_api_integration branch from dbef1e0 to 4d3f760 Compare June 17, 2021 06:35

iefremov reviewed Jun 17, 2021

View reviewed changes

components/brave_vpn/brave_vpn_service.h Show resolved Hide resolved

iefremov reviewed Jun 17, 2021

View reviewed changes

iefremov requested changes Jun 17, 2021

View reviewed changes

deeppandya force-pushed the vpn_api_integration branch 2 times, most recently from 72c8754 to f17b955 Compare June 25, 2021 16:27

iefremov reviewed Jun 29, 2021

View reviewed changes

iefremov self-requested a review June 29, 2021 13:56

deeppandya added 13 commits July 5, 2021 17:35

Add vpn api integration to components

a39b45a

Add api implementation for guardian

bd6cc06

Update name conventions

34b517d

Update deps

ADd misssing includes

14bda31

Fix include_rule error

ef9c76e

Fix lint error

13de84f

Update vpn deps

94a02fb

Refactor vpn files

0a3bbc2

Address comments on conversation

Address PR comments

8b69a03

Add check for record_v

8eeb8a6

Address PR comments

2de31ab

Fix gn_check error

4c836e3

Address PR comments

7ebd4a5

deeppandya force-pushed the vpn_api_integration branch from edf58dd to 7ebd4a5 Compare July 5, 2021 21:36

Add check for android to avoid error on other platforms

14dd0a1

iefremov reviewed Jul 6, 2021

View reviewed changes

iefremov approved these changes Jul 6, 2021

View reviewed changes

bsclifton approved these changes Jul 7, 2021

View reviewed changes

deeppandya merged commit 94f9b4f into master Jul 7, 2021

deeppandya deleted the vpn_api_integration branch July 7, 2021 16:35

bsclifton modified the milestones: 1.27.x - Beta, 1.28.x - Nightly Jul 7, 2021

bsclifton mentioned this pull request Jul 7, 2021

Vpn connection manager mac #9246

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brave Vpn api integration #9131

Brave Vpn api integration #9131

deeppandya commented Jun 15, 2021

simonhong Jun 15, 2021 •

edited

Loading

iefremov Jun 17, 2021

simonhong commented Jun 15, 2021

simonhong Jun 17, 2021

simonhong left a comment

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 17, 2021

iefremov Jun 29, 2021

iefremov Jun 29, 2021

deeppandya Jul 7, 2021

iefremov left a comment

iefremov Jun 29, 2021

iefremov Jun 29, 2021

iefremov Jun 29, 2021

iefremov Jun 29, 2021

iefremov Jun 29, 2021

iefremov commented Jun 29, 2021

iefremov Jul 6, 2021

bsclifton left a comment •

edited

Loading

		@@ -0,0 +1,3 @@
		include_rules = [
		"+services/network/public/cpp",


		class BraveVpnService;

		namespace chrome {

Brave Vpn api integration #9131

Brave Vpn api integration #9131

Conversation

deeppandya commented Jun 15, 2021

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simonhong Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jun 15, 2021

Choose a reason for hiding this comment

simonhong left a 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

iefremov left a 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

iefremov commented Jun 29, 2021

Choose a reason for hiding this comment

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

simonhong Jun 15, 2021 •

edited

Loading

bsclifton left a comment •

edited

Loading