-
Notifications
You must be signed in to change notification settings - Fork 891
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
separate alerting origin from download origin #27049
Conversation
if (url_identity.type == UrlIdentity::Type::kIsolatedWebApp) { | ||
return url_identity.name; | ||
} | ||
|
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.
took out the code above because i'm not sure we actually need it
- it only returns the name of an extension/app, not a proper message like "$app wants to download...", which seems like a bug
- i think the chromium version is just intended to return a human-friendly name for extensions/etc. it's nice to have but without it, probably we would just show the generic "this page" message, which is ok.
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.
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.
@goodov thanks for testing it out. i think we can live with that and open a follow up unless you want to address it in this PR?
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.
security wise i think this view is better because an extension/app could name themselves "google.com" or something
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.
would be nice to have it like Extension <...> wants to save
. For consistency a similar change should be done for "wants to open" dialog and also for apps(?). A follow up sounds reasonable here.
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.
opened brave/brave-browser#42987
Released in v1.75.116 |
address brave/brave-browser#42939
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
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: