Skip to content

Setting proxy for safe browsing requests - #108

Merged
bbondy merged 1 commit into
masterfrom
safebrowsing_proxy
May 22, 2018
Merged

bbondy merged 1 commit into
masterfrom
safebrowsing_proxy

Conversation

@jumde

@jumde jumde commented May 18, 2018

Copy link
Copy Markdown
Contributor

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

@bbondy bbondy left a comment

Copy link
Copy Markdown
Member

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

@bbondy bbondy left a comment •

Copy link
Copy Markdown
Member

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

@bbondy bbondy left a comment

Copy link
Copy Markdown
Member

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

@bbondy bbondy left a comment

Copy link
Copy Markdown
Member

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
Comment thread browser/net/BUILD.gn
source_set("net") {
configs += [ "//brave/build/geolocation" ]
configs += [
"//brave/build/geolocation",

@bridiver bridiver May 23, 2018 •

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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