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

[WIP] Support 3p APIs in chrome.identity without Google API keys #10103

Closed
wants to merge 1 commit into from

Conversation

mariospr
Copy link
Contributor

@mariospr mariospr commented Sep 15, 2021

DO NOT REVIEW / MERGE THIS PR. This was a quick PR I cooked up for @jumde to follow up on as a result of simply applying a prototype patch on thop of Chromium, but it was never meant for merging in its current form.

This patch replaces the chrome.identity API, which relies on 1p APIs,
with a version that relies on 3p APIs, necessary for some extensions
to work Brave after chrome.identity changed to only support 1p APIs.

Resolves brave/brave-browser#15754

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This patch replaces the chrome.identity API, which relies on 1p APIs,
with a version that relies on 3p APIs, necessary for some extensions
to work Brave after chrome.identity changed to only support 1p APIs.

Based on https://crrev.com/c/2704571
@goodov
Copy link
Member

goodov commented Oct 4, 2021

Is this a draft? We can't merge such huge patches, they should be converted to single-line #defines and/or chromium_src overrides.

@mariospr
Copy link
Contributor Author

mariospr commented Oct 4, 2021

@AlexeyBarabash This is not ready for review, this was a quick PR I cooked up for @jumde to follow up on as a result of simply applying a prototype patch on thop of Chromium, but it was never meant for merging in its current form.

@goodov
Copy link
Member

goodov commented Oct 4, 2021

@AlexeyBarabash This is not ready for review, this was a quick PR I cooked up for @jumde to follow up on as a result of simply applying a prototype patch on thop of Chromium, but it was never meant for merging in its current form.

would you mind adding WIP prefix to the PR name then?

@mariospr mariospr changed the title Support 3p APIs in chrome.identity without Google API keys [WIP] Support 3p APIs in chrome.identity without Google API keys Oct 4, 2021
@mariospr
Copy link
Contributor Author

mariospr commented Oct 4, 2021

@AlexeyBarabash Sure, done.

@AlexeyBarabash
Copy link
Contributor

@mariospr
seems you meant @goodov instead of me :)

@mariospr
Copy link
Contributor Author

mariospr commented Oct 5, 2021

@mariospr seems you meant @goodov instead of me :)

Oops! You're right, sorry for the noise

@mariospr
Copy link
Contributor Author

This has stalled, please reopen if needed

@mariospr mariospr closed this Mar 25, 2022
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.

Setting "Allow Google login for extensions" does not work
4 participants