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

Use Brave-specific labels for os_crypt key #61

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Conversation

garrettr
Copy link
Contributor

I used this codesearch to find all apparent mentions of "(C|c)hrom(e|ium)" labels used by the os_crypt backend and replaced them with Brave-specific labels. Brave doesn't have a Chrome vs. Chromium-style branding divide, so I removed the corresponding preprocessor conditionals as necessary.

If there's a better way to accomplish this other than adding a bunch of small patches, please let me know!

Resolves https://github.com/brave/brave/issues/110.

@garrettr garrettr requested review from bbondy and darkdh March 21, 2018 00:23

namespace {

-#if defined(GOOGLE_CHROME_BUILD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we want these patches to be as small as possible for easier maintainability. There's no reason to remove the GOOGLE_CHROME_BUILD define because that will never be hit. We should probably have our own flag like in muon. Maybe BRAVE_BUILD, but in any case for this commit we only want to update chromium to brave

Copy link
Collaborator

Choose a reason for hiding this comment

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

and I think there may be a second issue for the chrome importer hack that @darkdh can explain

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

and importer hack should enumerate chromium and chrome
https://github.com/brave/muon/pull/227/files#diff-85ff0d997b1a2f089546f7b2823b0ad7R1023
because we used to share same safe storage as chromium

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridiver I feel it is confusing to leave conditionals that we know "will never be hit" in, but appreciate that you prefer to keep unnecessary changes in the patches to a minimum. I will restore the unused conditionals and focus on keeping the patches small.


namespace {

-#if defined(GOOGLE_CHROME_BUILD)
Copy link
Member

Choose a reason for hiding this comment

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


namespace {

-#if defined(GOOGLE_CHROME_BUILD)
Copy link
Member

Choose a reason for hiding this comment

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

and importer hack should enumerate chromium and chrome
https://github.com/brave/muon/pull/227/files#diff-85ff0d997b1a2f089546f7b2823b0ad7R1023
because we used to share same safe storage as chromium

@garrettr garrettr force-pushed the os_crypt-key-label branch from 3822367 to fcb9e98 Compare March 21, 2018 23:39
@garrettr
Copy link
Contributor Author

Refactored to take into account @bridiver and @darkdh's review comments. Please re-review.

@darkdh I decided not to include the "chrome importer hack" in this PR, to keep it focused. I will include it in my upcoming "import passwords from Chrome" PR instead, unless you think it would be best to include it in this one.

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.

++

@darkdh darkdh merged commit 91fd459 into master Mar 22, 2018
@bsclifton bsclifton deleted the os_crypt-key-label branch June 18, 2018 06:29
NejcZdovc added a commit that referenced this pull request Dec 10, 2018
bbondy added a commit that referenced this pull request Feb 18, 2019
actually write to the output dir so we don't rebuild every time
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