Remove chrome/browser/ash/display/quirks_manager_delegate_impl [chromium/src : main]
Hidehiko Abe (Gerrit)
Hidehiko Abe added 3 comments![]( "Open in Gerrit")
friend class DisplayColorManagerTest;NOTE: currently there's no way to set up global CrosSettings instance outside of //chrome.
I'm working on making it possible, but that'll need a bit more changes still.
This should be short term work around until that's done.
assert(is_chromeos, "Quicks can be used only in chromeos")Clarification: Looks like this is only for chromeos now.
Is there any plan to enable it for other platforms?
If not, maybe move this to chromeos/ash/components/quirks?
Related details
- Mitsuru Oshima
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
assert(is_chromeos, "Quicks can be used only in chromeos")Clarification: Looks like this is only for chromeos now.
Is there any plan to enable it for other platforms?If not, maybe move this to chromeos/ash/components/quirks?
It's up to us. There are "chromeos only" components under components, such as user_manager, exo, and moving all of them is probably not important as long as it's clearly declared like this (but possbiile).
"//chromeos/ash/components/settings",Hmm, I feel odd if this depeneds on settings. Can't we just inject callbacks?
Related details
- Hidehiko Abe
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 3 comments![]( "Open in Gerrit")
Thank you for review. PTAL.
FYI: Greg.
assert(is_chromeos, "Quicks can be used only in chromeos")Mitsuru OshimaClarification: Looks like this is only for chromeos now.
Is there any plan to enable it for other platforms?If not, maybe move this to chromeos/ash/components/quirks?
It's up to us. There are "chromeos only" components under components, such as user_manager, exo, and moving all of them is probably not important as long as it's clearly declared like this (but possbiile).
Thanks for reply.
Moving the components is just an option, and indeed low priority. I just wanted to confirm this is only for chromeos (as this introduces chromeos only code to this module).
Hmm, I feel odd if this depeneds on settings. Can't we just inject callbacks?
- Mitsuru Oshima
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Levin (Gerrit)
Greg Levin added 1 comment![]( "Open in Gerrit")
Thank you for review. PTAL.
FYI: Greg.
Thanks for the ping. But I work on Privacy these days, and haven't touched ChromeOS code in over 7 years(!). So I'm not really qualified to evaluate a change of this magnitude. Glad Oshima's taking a look :)
Related details
- Hidehiko Abe
- Mitsuru Oshima
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
2 comments
if (!base::SysInfo::IsRunningOnChromeOS()) {optional:
#if !BUILDFLAG(IS_CHROMEOS_DEVICE)
to exclude non production code.
quirks_policy_controller_.reset();It's less intuitive to reset then call shutdown related classes. Can you elaborate this dependency in the comment?
Related details
- Hidehiko Abe
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Commit-Queue | +2 |
3 comments
optional:
#if !BUILDFLAG(IS_CHROMEOS_DEVICE)
to exclude non production code.
Done
It's less intuitive to reset then call shutdown related classes. Can you elaborate this dependency in the comment?
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.cc
Insertions: 15, Deletions: 14.
The diff is too large to show. Please review the diff.
```
Change information
Remove chrome/browser/ash/display/quirks_manager_delegate_impl
The impl is living in //chrome. However, the actual dependency is
only user dir path injection for linux-chromeos.
That can be handled in chromeos-chrome start up in a way to override
an entry in PathService.
BUG=422082022
TEST=Tryjob
- M ash/display/display_color_manager_unittest.cc
- M chrome/browser/ash/display/BUILD.gn
- M chrome/browser/ash/display/DEPS
- M chrome/browser/ash/display/quirks_browsertest.cc
- D chrome/browser/ash/display/quirks_manager_delegate_impl.cc
- D chrome/browser/ash/display/quirks_manager_delegate_impl.h
- M chrome/browser/ash/main_parts/BUILD.gn
- M chrome/browser/ash/main_parts/DEPS
- M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.cc
- M chrome/browser/ash/main_parts/chrome_browser_main_parts_ash.h
- M chrome/browser/ash/policy/handlers/device_quirks_policy_browsertest.cc
- A chromeos/ash/experiences/policy/handlers/quirks/BUILD.gn
- A chromeos/ash/experiences/policy/handlers/quirks/DEPS
- A chromeos/ash/experiences/policy/handlers/quirks/quirks_policy_controller.cc
- A chromeos/ash/experiences/policy/handlers/quirks/quirks_policy_controller.h
- M components/quirks/BUILD.gn
- M components/quirks/DEPS
- M components/quirks/quirks_client.cc
- M components/quirks/quirks_client.h
- M components/quirks/quirks_manager.cc
- M components/quirks/quirks_manager.h
Code-Review: +1 by Mitsuru Oshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |