Add Brave geolocations and system network redirect #19

bbondy · 2018-01-12T04:41:29Z

Fix for https://github.com/brave/brave/issues/48

bridiver · 2018-01-12T06:41:12Z

common/BUILD.gn

@@ -1,4 +1,7 @@
 source_set("common") {
+  #public_configs = [


bridiver · 2018-01-12T06:44:18Z

patches/device-geolocation-BUILD.gn.patch

+
+ component("geolocation") {
+   defines = [ "DEVICE_GEOLOCATION_IMPLEMENTATION" ]
+  configs += [ "//brave/build/geolocation" ]


is this file already in the repo?

No I just forgot to add it to git, will push it out in a sec.

bridiver · 2018-01-12T06:45:05Z

common/google_api_key.h

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_COMMON_GOOGLE_API_KEY_H_


do we actually need this file? We could just do a define in the gn config

gn only sounds good, this is the way it was in muon so just used that.

bridiver · 2018-01-12T06:46:46Z

patches/device-geolocation-network_location_request.cc.patch

+
+-const char kNetworkLocationBaseUrl[] =
+-    "https://www.googleapis.com/geolocation/v1/geolocate";
+const char kNetworkLocationBaseUrl[] = GOOGLEAPIS_ENDPOINT;


I guess the define must already be in the config? I think the google_api_key header was just leftover from electron and never got removed from muon

it's in the build config which I'll push up.

bbondy · 2018-01-12T19:00:40Z

@bridiver ready for re-review, the only patch now is to use the brave network delegate for system requests.

bbondy · 2018-01-13T04:27:03Z

Ready for review again, this creates a different network delegate for system and profile and uses a common base class so we can chain different work items in each.

bridiver · 2018-01-13T19:16:25Z

browser/net/brave_static_redirect_network_delegate_work.cc

+  std::shared_ptr<OnBeforeURLRequestContext> ctx) {
+
+  base::StringPiece value(request->url().spec());
+  if (value.starts_with("https://www.googleapis.com/geolocation/v1/geolocate?key=")) {


this is ok, but I think you might want to use URLPattern here

bridiver · 2018-01-13T19:17:02Z

browser/net/brave_static_redirect_network_delegate_work.cc

+
+  base::StringPiece value(request->url().spec());
+  if (value.starts_with("https://www.googleapis.com/geolocation/v1/geolocate?key=")) {
+    *new_url = GURL(GOOGLEAPIS_ENDPOINT GOOGLEAPIS_API_KEY);


I think our key should be correct because that's set in one of the delegates

We no longer set the key because it's not needed, so no harm to do it here.

bridiver · 2018-01-13T19:17:56Z

browser/net/BUILD.gn

+    "brave_httpse_network_delegate_work.h",
+    "brave_profile_network_delegate.cc",
+    "brave_profile_network_delegate.h",
+    "brave_static_redirect_network_delegate_work.cc",


I'll change to _helper mainly it is not a delegate itself but just a function that's stored as a callback in a vector to execute.

bridiver

lgtm

bridiver · 2018-01-14T05:07:18Z

browser/net/brave_static_redirect_network_delegate_helper.h

+    GURL* new_url,
+    const ResponseCallback& next_callback,
+    std::shared_ptr<OnBeforeURLRequestContext> ctx);
+


I think we must be missing lint checking?

we still don't have lint checking

Fix for no unsignedTx object on reconcile

Add settings for cookies and referrer blocking

* Add dockerfile for building * Update Dockerfile and README

bbondy self-assigned this Jan 12, 2018

bbondy requested a review from bridiver January 12, 2018 04:41

bridiver reviewed Jan 12, 2018

View reviewed changes

common/BUILD.gn Outdated

@@ -1,4 +1,7 @@

source_set("common") {

#public_configs = [

Copy link

Collaborator

bridiver Jan 12, 2018

Choose a reason for hiding this comment

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

?

bridiver reviewed Jan 12, 2018

View reviewed changes

bbondy force-pushed the geolocations-master branch 4 times, most recently from f2f2349 to a2ee013 Compare January 12, 2018 18:59

bbondy force-pushed the geolocations-master branch from a2ee013 to 05f7c23 Compare January 13, 2018 04:21

bbondy changed the title ~~Add Brave geolocations support for our build config~~ Jan 13, 2018

bbondy changed the title ~~Add Brave geolocations support and system network redirect support~~ Jan 13, 2018

bridiver reviewed Jan 13, 2018

View reviewed changes

bridiver previously approved these changes Jan 13, 2018

View reviewed changes

Add Brave geolocations support for our build config

b45174a

bbondy dismissed bridiver’s stale review via b45174a January 14, 2018 03:31

bbondy force-pushed the geolocations-master branch from 05f7c23 to b45174a Compare January 14, 2018 03:31

bbondy merged commit 4c84689 into master Jan 14, 2018

bridiver reviewed Jan 14, 2018

View reviewed changes

bsclifton deleted the geolocations-master branch June 18, 2018 06:25

NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018

Merge pull request #19 from brave-intl/json_reader_crash

b88a4a1

Fix for no unsignedTx object on reconcile

bbondy added a commit that referenced this pull request Feb 18, 2019

Merge pull request #19 from brave/cookies

e98aed2

Add settings for cookies and referrer blocking

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Add Dockerfile for building (#19)

03703a6

* Add dockerfile for building * Update Dockerfile and README

This was referenced Feb 19, 2023

Pass "production" as vpn payment env value in prod env #17244

Merged

Always Show Full URLs option appears twice -> fixed #17286

Merged

stephendonner mentioned this pull request Feb 21, 2023

Renamed Bookmark -> Bookmarks #17274

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Brave geolocations and system network redirect #19

Add Brave geolocations and system network redirect #19

bbondy commented Jan 12, 2018

bridiver Jan 12, 2018

bridiver Jan 12, 2018

bbondy Jan 12, 2018

bridiver Jan 12, 2018

bbondy Jan 12, 2018

bridiver Jan 12, 2018

bbondy Jan 12, 2018

bbondy commented Jan 12, 2018

bbondy commented Jan 13, 2018

bridiver Jan 13, 2018

bbondy Jan 14, 2018

bridiver Jan 13, 2018

bbondy Jan 14, 2018

bridiver Jan 13, 2018

bbondy Jan 14, 2018

bridiver left a comment

bridiver Jan 14, 2018

bbondy Jan 14, 2018

Add Brave geolocations and system network redirect #19

Add Brave geolocations and system network redirect #19

Conversation

bbondy commented Jan 12, 2018

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

bbondy commented Jan 12, 2018

bbondy commented Jan 13, 2018

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

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment