-
Notifications
You must be signed in to change notification settings - Fork 879
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
Allow permission requests within the same eTLD+1 #13564
Conversation
components/permissions/contexts/brave_wallet_permission_context.cc
Outdated
Show resolved
Hide resolved
514f0c0
to
854ef4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, only one nit left.
EXPECT_FALSE(responses.empty()); | ||
})); | ||
|
||
content::RunAllTasksUntilIdle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use base::RunLoop
instead and call Quit
in the lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3e1a254
})); | ||
|
||
content::RunAllTasksUntilIdle(); | ||
EXPECT_FALSE(IsPendingGroupedRequestsEmpty(cases[i].type)) << "case: " << i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can also write it like this, seems okay to distinguish cases by the wallet address:
for (const auto& case : cases) {
SCOPED_TRACE(testing::PrintToString(case.addresses));
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about that, thanks!
Fixed in 3e1a254
854ef4a
to
9d0ff1f
Compare
6a55c3e
to
3e1a254
Compare
FYI - I removed some debug prints and updated comments to reference the latest hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
3e1a254
to
df1ef78
Compare
original_requesting_origin == url::Origin::Create(embedding_origin)) { | ||
net::registry_controlled_domains::SameDomainOrHost( | ||
original_requesting_origin, url::Origin::Create(embedding_origin), | ||
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wanted to double check - embedding_origin
is the top level (main frame) origin as opposed to the parent origin? ex: if a.com embeds b.com which embeds c.com and the requesting origin is c.com, embedding origin is a.com not b.com.
Closing in favor of #13783 |
Resolves brave/brave-browser#23142
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: