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.

Sign up for GitHub

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

Jump to bottom

Refactor tor to brave/components #6881

Merged
merged 10 commits into from
Oct 28, 2020
Merged

Refactor tor to brave/components #6881

merged 10 commits into from
Oct 28, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Oct 16, 2020 •
edited
Loading

Resolves brave/brave-browser#1114

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@darkdh darkdh force-pushed the components-tor branch 2 times, most recently from 2794dd6 to 36eda0f Compare October 21, 2020 21:07
@darkdh darkdh self-assigned this Oct 21, 2020
@darkdh darkdh marked this pull request as ready for review October 21, 2020 21:56
@darkdh darkdh requested a review from a team as a code owner October 21, 2020 21:56
@darkdh darkdh requested review from simonhong and bbondy October 21, 2020 21:57
@darkdh darkdh force-pushed the components-tor branch 3 times, most recently from 2756623 to c24c61d Compare October 23, 2020 22:33
simonhong
simonhong reviewed Oct 26, 2020
View reviewed changes
browser/tor/brave_tor_client_updater_delegate.h Outdated
explicit BraveTorClientUpdaterDelegate(const base::FilePath& user_data_dir);
~BraveTorClientUpdaterDelegate() override;

// BraveTorClientUpdater::Delegate
Copy link
Member

@simonhong simonhong Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think tor component can do cleanup and check its disabled state by passing PrefService(local_state()) and user data dir to component instead of using Delegate. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I also think this delegate isn't needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 2025088c945de7d44c240623355c7ececb4d6786

bbondy
bbondy reviewed Oct 26, 2020
View reviewed changes
chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc
@@ -115,9 +115,9 @@ void BraveRenderViewContextMenu::AddSpellCheckServiceItem(
void BraveRenderViewContextMenu::InitMenu() {
RenderViewContextMenu_Chromium::InitMenu();

int index = -1;
Copy link
Member

Choose a reason for hiding this comment

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

This might give an unused local variable warning (or error because I think it's treated as an error) under certain conditions.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in 2025088c945de7d44c240623355c7ececb4d6786

@bbondy
Copy link
Member

bbondy commented Oct 26, 2020

lgtm, but I think it'd be good to get rid of the delegate if it's not needed.

simonhong
simonhong reviewed Oct 27, 2020
View reviewed changes
components/tor/BUILD.gn Outdated
}

deps += [
"//brave/app:brave_generated_resources_grit",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is layer violoation. If tor components needs resources, it should be included in component resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in e1726d757df0524a90d1c7a961847bebc7ed8eee

simonhong
simonhong reviewed Oct 27, 2020
View reviewed changes
components/tor/tor_profile_service.cc Outdated

namespace tor {

TorProfileService::TorProfileService() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: = default

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in e1726d757df0524a90d1c7a961847bebc7ed8eee

simonhong
simonhong reviewed Oct 27, 2020
View reviewed changes
components/tor/tor_profile_service.cc Outdated
TorProfileService::TorProfileService() {
}

TorProfileService::~TorProfileService() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: = default

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in e1726d757df0524a90d1c7a961847bebc7ed8eee

simonhong
simonhong reviewed Oct 27, 2020
View reviewed changes
components/tor/tor_switches.h Outdated

namespace tor {

extern const char kDisableTorClientUpdaterExtension[];
Copy link
Member

Choose a reason for hiding this comment

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

How about using constexpr instead of having separated definitions and declaration.
It would be good for compile time optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in e1726d757df0524a90d1c7a961847bebc7ed8eee

simonhong
simonhong reviewed Oct 27, 2020
View reviewed changes
test/BUILD.gn Outdated
@@ -721,6 +711,15 @@ if (!is_android) {
]
}

if (enable_tor) {
sources += [
"//brave/browser/brave_local_state_browsertest.cc",
Copy link
Member

@simonhong simonhong Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about moving this test into //brave/browser/tor:browser_tests or unit_test?
This test only checks tor disabled state and I think it would be better to unify related feature's test cases in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in e1726d757df0524a90d1c7a961847bebc7ed8eee

@simonhong
Copy link
Member

I don't have more comments. Great refactoring 👍

@darkdh darkdh requested review from simonhong and bbondy October 27, 2020 23:16
simonhong
simonhong approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

mkarolin
mkarolin approved these changes Oct 28, 2020
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

chromium_src/patches LGTM

@darkdh
Copy link
Member Author

darkdh commented Oct 28, 2020

Known issues

01:18:10  1 test failed:
01:18:10      RewardsContributionBrowserTest.PanelMonthlyTipAmount
(../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:927)

https://ci.brave.com/job/pr-brave-browser-components-tor-macos/11/execution/node/249/log/

00:58:53  1 test failed:
00:58:53      RewardsContributionBrowserTest.RecurringTipForUnverifiedPublisher 
(../../brave/components/brave_rewards/browser/test/rewards_contribution_browsertest.cc:327)

https://ci.brave.com/job/pr-brave-browser-components-tor-linux/11/execution/node/249/log/

@darkdh darkdh merged commit bb075d5 into master Oct 28, 2020
@darkdh darkdh deleted the components-tor branch October 28, 2020 16:30
@darkdh darkdh added this to the 1.18.x - Nightly milestone Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkarolin mkarolin mkarolin approved these changes

@simonhong simonhong simonhong approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@darkdh darkdh

Labels
None yet
Projects
None yet
Milestone
1.18.x - Release
Development

Successfully merging this pull request may close these issues.

Move tor to /components
4 participants
@darkdh @bbondy @simonhong @mkarolin