-
Notifications
You must be signed in to change notification settings - Fork 813
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
Fixes a crash on Android permission dialog after cr125 update #23622
Conversation
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
#define kStorageAccess \ | ||
kStorageAccess, kWidevine, kBraveEthereum, kBraveSolana, \ | ||
kBraveGoogleSignInPermission, kBraveLocalhostAccessPermission, \ | ||
kBraveDefault |
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.
this doesn't seem right to me cc @goodov
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'm not sure why we're getting the default content setting here in permissions, but it's also not a brave specific permission so we shouldn't require a new request type here. I think we must be handling something incorrectly. Can this be replicated on desktop under the same conditions?
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.
the crash is just uma stuff, but it seems like there is some other underlying issue if we're getting DEFAULT as the dismissal type. I feel like this change might just be covering up a more serious problem. In particular since DEFAULT is not a brave-specific type, if it was expected it would be handled by chromium. The fact that we're seeing it at all seems to indicate that this is just a symptom of the real problem
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.
we are getting DEFAULT as we need to register Widevine Content Settings Type here otherwise https://github.com/brave/brave-core/blob/master/chromium_src/components/content_settings/core/common/content_settings_types.mojom and here https://github.com/brave/brave-core/blob/master/chromium_src/components/content_settings/core/browser/content_settings_registry.cc, so the conversion from Content Settings Type to Registry Type for Widevine works. I don't think we want that as you mentioned it's only for UMA which we don't need.
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 am open for suggestions on how we can resolve it different and fast without delaying cr126 update.
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.
chromium didn't have that UMA check before cr126 that's why it worked without any issues.
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 see what's going on here now, we don't have a content setting for widevine so it's mapping to ContentSettingsType::DEFAULT
. As discussed for now let's just map DEFAULT
<-> kWidevine
and we'll follow-up to figure out if we want to keep that, add a widevine content setting or do something else
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 created a follow to think about that brave/brave-browser#38304
@@ -39,6 +39,7 @@ constexpr auto& kMicIconValue = vector_icons::kMicIcon; | |||
case RequestType::kBraveSolana: \ | |||
case RequestType::kBraveGoogleSignInPermission: \ | |||
case RequestType::kBraveLocalhostAccessPermission: \ | |||
case RequestType::kBraveDefault: \ |
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.
yeah, right, I removed it.
e0e5517
to
a4af12f
Compare
a4af12f
to
27649d9
Compare
Verification PASSED on
Using the STR/Cases outlined via brave/brave-browser#38297 (comment), reproduced the original issue using screen-20240514-165734.mp4Using the same STR/Cases mentioned above, ensured that screen-20240514-170200.mp4 |
Resolves brave/brave-browser#38297
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:
Described in the original issue.