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

Use Brave Importer classes for auto-import #844

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Use Brave Importer classes for auto-import #844

merged 1 commit into from
Nov 12, 2018

Conversation

garrettr
Copy link
Contributor

@garrettr garrettr commented Nov 9, 2018

Resolves brave/brave-browser#2013.
Resolves brave/brave-browser#2014.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Follow test plans for brave/brave-browser#2013, brave/brave-browser#2014. Cookies and stats should be imported, where previously they were not.

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

darkdh
darkdh reviewed Nov 9, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

ah, my bad for previous #729 review because I forgot we have to use BraveExternalProcessImporterHost

LGTM, shouldn't this merge into master and cherry-pick into 0.56.x?
The current target branch is 0.56.x

darkdh
darkdh requested changes Nov 9, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

Actually why not just overriding ExternalProcessImporterHost to BraveExternalProcessImporterHost and ProfileWriter to BraveProfileWriter.

@garrettr garrettr changed the title Use Brave Importer classes consistently for auto-import Use Brave Importer classes for auto-import Nov 12, 2018
@garrettr garrettr changed the base branch from 0.56.x to master November 12, 2018 00:07
@garrettr
Copy link
Contributor Author

@darkdh Implemented both of your review comments; ready for re-review.

I've tested and confirmed this PR fixes both issues on both 0.56.x and master. On 0.57.x and above, #886 is also a prerequisite.

darkdh
darkdh approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

@garrettr garrettr merged commit b50a2ab into master Nov 12, 2018
garrettr added a commit that referenced this pull request Nov 12, 2018
@garrettr
Merge pull request #844 from brave/issue/2013
31de91a 
Use Brave Importer classes for auto-import
garrettr added a commit that referenced this pull request Nov 12, 2018
@garrettr
Merge pull request #844 from brave/issue/2013
4993335 
Use Brave Importer classes for auto-import
garrettr added a commit that referenced this pull request Nov 12, 2018
@garrettr
Merge pull request #844 from brave/issue/2013
fd9849f 
Use Brave Importer classes for auto-import
@garrettr
Copy link
Contributor Author

master: b50a2ab
0.58.x: 31de91a
0.57.x: 4993335
0.56.x: fd9849f

@NejcZdovc NejcZdovc deleted the issue/2013 branch November 13, 2018 04:45
mbacchi added a commit that referenced this pull request Nov 14, 2018
@mbacchi
Revert "Merge pull request #844 from brave/issue/2013"
930a507 
This reverts commit fd9849f.

Reverting because we are doing a RC build for 0.56.x and did not
intend this to be in the RC milestone.
@mbacchi
Copy link
Contributor

mbacchi commented Nov 14, 2018

Reverted 0.56.x commit fd9849f with 930a507 in order to create a limited scope release channel RC.

mbacchi added a commit that referenced this pull request Nov 14, 2018
@mbacchi
Revert "Revert "Merge pull request #844 from brave/issue/2013""
68bc897 
This reverts commit 930a507.
@mbacchi
Copy link
Contributor

mbacchi commented Nov 14, 2018

Added back at the request of Garrett and Brian Bondy by reverting my revert with 68bc897

@bbondy bbondy added this to the 0.56.x - Release milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@bridiver bridiver Awaiting requested review from bridiver

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@garrettr garrettr

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

Successfully merging this pull request may close these issues.

Stats not imported during auto-upgrade cookies not imported - follow up to 1545
4 participants
@garrettr @mbacchi @darkdh @bbondy