-
Notifications
You must be signed in to change notification settings - Fork 921
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.
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
Add Brave geolocations and system network redirect #19
Conversation
common/BUILD.gn
Outdated
@@ -1,4 +1,7 @@ | |||
source_set("common") { | |||
#public_configs = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
|
||
component("geolocation") { | ||
defines = [ "DEVICE_GEOLOCATION_IMPLEMENTATION" ] | ||
+ configs += [ "//brave/build/geolocation" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this file already in the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I just forgot to add it to git, will push it out in a sec.
common/google_api_key.h
Outdated
* 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_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we actually need this file? We could just do a define in the gn config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gn only sounds good, this is the way it was in muon so just used that.
|
||
-const char kNetworkLocationBaseUrl[] = | ||
- "https://www.googleapis.com/geolocation/v1/geolocate"; | ||
+const char kNetworkLocationBaseUrl[] = GOOGLEAPIS_ENDPOINT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's in the build config which I'll push up.
f2f2349
to
a2ee013
Compare
@bridiver ready for re-review, the only patch now is to use the brave network delegate for system requests. |
a2ee013
to
05f7c23
Compare
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. |
std::shared_ptr<OnBeforeURLRequestContext> ctx) { | ||
|
||
base::StringPiece value(request->url().spec()); | ||
if (value.starts_with("https://www.googleapis.com/geolocation/v1/geolocate?key=")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok, but I think you might want to use URLPattern here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
|
||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our key should be correct because that's set in one of the delegates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer set the key because it's not needed, so no harm to do it here.
browser/net/BUILD.gn
Outdated
"brave_httpse_network_delegate_work.h", | ||
"brave_profile_network_delegate.cc", | ||
"brave_profile_network_delegate.h", | ||
"brave_static_redirect_network_delegate_work.cc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_work
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
05f7c23
to
b45174a
Compare
GURL* new_url, | ||
const ResponseCallback& next_callback, | ||
std::shared_ptr<OnBeforeURLRequestContext> ctx); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we must be missing lint checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Fix for https://github.com/brave/brave/issues/48