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.

Sign up for GitHub

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

Jump to bottom

[CR115] Investigate whether AcceleratorServiceFactory should/shouldn't allow guest profiles #31025

Closed
emerick opened this issue Jun 13, 2023 · 2 comments · Fixed by brave/brave-core#23644
Closed

[CR115] Investigate whether AcceleratorServiceFactory should/shouldn't allow guest profiles #31025

emerick opened this issue Jun 13, 2023 · 2 comments · Fixed by brave/brave-core#23644
Assignees
@emerick @cdesouza-chromium @mkarolin
Labels
Chromium/Followup Followup work logged during Chromium rebase dev-concern OS/Desktop QA/No release-notes/exclude
Milestone
1.68.x - Nightly

Comments

@emerick
Copy link
Contributor

emerick commented Jun 13, 2023 •
edited

This came up during the CR115 review and was noticed by @goodov. We should investigate whether AcceleratorServiceFactory shouldn't allow guest profiles (the WithGuest line here):

AcceleratorServiceFactory::AcceleratorServiceFactory()
    : ProfileKeyedServiceFactory(
          "AcceleratorServiceFactory",
          ProfileSelections::Builder()
              .WithRegular(ProfileSelection::kRedirectedToOriginal)
              .WithGuest(ProfileSelection::kRedirectedToOriginal)
              .Build()) {}

When I removed that line, however, it led to several Brave and upstream test failures. There seemed to be two main issues:

  • BraveBrowserView performs a ScopedObservation of AccelatorService::Observer, but that will be null in guest sessions
  • The vertical tabs feature attempts to retrieve accelerators in guest sessions
@emerick emerick self-assigned this Jun 13, 2023
@emerick emerick mentioned this issue Jun 13, 2023
Merged
25 tasks
@goodov
Copy link
Member

goodov commented Jun 14, 2023

tbh I'm not 100% sure if AcceleratorServiceFactory should or should not exist in Guest profile.

We might just need to have it enabled to support hotkeys in all profiles if that's the main use case of this service.

@emerick emerick changed the title AcceleratorServiceFactory shouldn't allow guest profiles Investigate whether AcceleratorServiceFactory should/shouldn't allow guest profiles Jun 14, 2023
@emerick
Copy link
Contributor Author

emerick commented Jun 14, 2023 •
edited

Thanks, I updated this issue to reflect that this requires some investigation to determine the appropriate course of action.

@emerick emerick changed the title Investigate whether AcceleratorServiceFactory should/shouldn't allow guest profiles [CR115] Investigate whether AcceleratorServiceFactory should/shouldn't allow guest profiles Feb 9, 2024
@emerick emerick added the Chromium/Followup Followup work logged during Chromium rebase label Feb 9, 2024
@mkarolin mkarolin mentioned this issue May 14, 2024
Merged
24 tasks
@mkarolin mkarolin added this to the 1.68.x - Nightly milestone May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emerick emerick

@cdesouza-chromium cdesouza-chromium

@mkarolin mkarolin

Labels
Chromium/Followup Followup work logged during Chromium rebase dev-concern OS/Desktop QA/No release-notes/exclude
Projects
None yet
Milestone
1.68.x - Nightly
Development

Successfully merging a pull request may close this issue.

Do not create AcceleratorService for Guest profile. brave/brave-core
4 participants
@emerick @goodov @cdesouza-chromium @mkarolin