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.

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

Setting proxy for safe browsing requests #108

Merged
merged 1 commit into from
May 22, 2018
Merged

Conversation

jumde
Copy link
Contributor

@jumde jumde commented May 18, 2018

@jumde jumde requested a review from bbondy May 18, 2018 08:18
bbondy
bbondy previously requested changes May 20, 2018
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Instead let's avoid the patch file by modifying the network traffic like we do for geolocations and updates here:

browser/net/brave_static_redirect_network_delegate_helper.cc

Thanks!

@jumde jumde force-pushed the safebrowsing_proxy branch 3 times, most recently from fc34341 to 1dc00a9 Compare May 21, 2018 20:49
@jumde jumde dismissed bbondy’s stale review May 21, 2018 20:54

Updated per comments

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Please add a unit test for this in browser/net/brave_static_redirect_network_delegate_helper_unittest.cc
Thanks!

@jumde jumde force-pushed the safebrowsing_proxy branch from 1dc00a9 to 8cf5724 Compare May 22, 2018 19:30
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Pls make the pattern not include v4 because when v5 happens it wouldn't match and we might miss it. Then add test for v5 too please if you don't mind. Thank you!

@jumde jumde force-pushed the safebrowsing_proxy branch 2 times, most recently from f5be645 to c4dffff Compare May 22, 2018 21:04
@jumde jumde force-pushed the safebrowsing_proxy branch from c4dffff to 8e17506 Compare May 22, 2018 21:05
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

🎉

@bbondy bbondy merged commit 7698bd0 into master May 22, 2018
@@ -1,7 +1,10 @@
import("//build/config/features.gni")

source_set("net") {
configs += [ "//brave/build/geolocation" ]
configs += [
"//brave/build/geolocation",
Copy link
Collaborator

@bridiver bridiver May 23, 2018

Choose a reason for hiding this comment

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

I'm not sure that using configs like this is the best way to do this. I think it would be better to create a normal dependency //brave/browser/safe_browsing and give it a public config. That would be a more typical pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw - my reasoning for this is that a dep provides more flexibility to manage configs, sources and add any additional deps that might be required in the future. In most cases (there are some exceptions), configs are treated more like internal implementation details for deps and those details are passed along using public_configs

@bsclifton bsclifton deleted the safebrowsing_proxy branch June 18, 2018 06:28
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants