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

Android component updater #2484

Merged
merged 13 commits into from
May 24, 2019
Merged

Android component updater #2484

merged 13 commits into from
May 24, 2019

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented May 22, 2019

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.

@bridiver bridiver self-assigned this May 22, 2019
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

I was able to review code only. I'm not very familiar with the logic. So better to have an another reviewer as well.

base::Bind(base::IgnoreResult(
&OnBeforeURLRequestDispatchOnIOThread), next_callback, ctx));
// TODO(bridiver) - fix this
OnBeforeURLRequestAdBlockTP(ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on what you want to fix here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually the comment can just be removed, I had that there as a reminder from before our conversation

@bridiver bridiver force-pushed the android-component-updater branch from 976fa95 to 396a587 Compare May 23, 2019 17:58
@bridiver bridiver force-pushed the android-component-updater branch from fb77ddd to 4d5a958 Compare May 23, 2019 18:55
@bridiver bridiver marked this pull request as ready for review May 23, 2019 19:14
@bridiver bridiver force-pushed the android-component-updater branch from 4d5a958 to 886dc91 Compare May 23, 2019 20:36
@bridiver bridiver requested a review from darkdh May 23, 2019 21:01
}

void TrackingProtectionService::UpdateFirstPartyStorageTrackers(
std::vector<std::string>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

std::vector<std::string> storage_trackers

if (storage_trackers_buffer_.empty()) {
LOG(ERROR) << "Could not obtain tracking protection data";
void TrackingProtectionService::OnGetSTPDATFileData(std::string contents) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, since this is on main thread now.

@@ -30,6 +30,10 @@ namespace brave_shields {
// checking and init.
class AdBlockBaseService : public BaseBraveShieldsService {
public:
using GetDATFileDataResult =
Copy link
Member

Choose a reason for hiding this comment

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

can we also have template< typename T > using GetDATFileDataResult = std::pair<std::unique_ptr<T>, brave_component_updater::DATFileDataBuffer> in components/brave_component_updater/browser/dat_file_util.h ?
also apply to AutoplayWhitelistService, ExtensionWhitelistService and TrackingProtectionService

@bridiver bridiver dismissed SergeyZhukovsky’s stale review May 24, 2019 19:27

he resolved them, not sure why this is still saying he requested changes

@bridiver bridiver merged commit e22053f into master May 24, 2019
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.

6 participants