Better define ChromeBrowserMainExtraPartsAsh, ChromeBrowserMainPartsChromeOS, and AshInit |
||||
Issue descriptionCurrently we have: * ChromeBrowserMainPartsChromeOS * ChromeBrowserMainExtraPartsAsh * AshInit The separation of responsibility is not especially clear and causes more confusion than modularization. We should do the integration carefully and thoughtfully and maintain modules where they make sense (ala DBusServices). We also need to move initialization with Shell dependencies to a Shell observer that implements OnShellInitialized since Shell initialization will not be synchronous in mustash since there are unavoidable local state dependencies.
,
Jan 2 2018
In an initail CL for this: https://chromium-review.googlesource.com/c/chromium/src/+/835349 Concern was expressed about the additional dependencies introduced to chrome/browser/chromeos/chrome_browser_main_chromeos.cc on chrome/browser/ui. As an alternate approach, we could: 1. Move current code in ChromeBrowserMainPartsChromeOS that depends on c/b/ui -> ChromeBrowserMainExtraPartsAsh (only 4 dependencies). 2. Add a DEPS rule to exclude c/b/ui from ChromeBrowserMainPartsChromeOS. 3. Keep ChromeBrowserMainExtraPartsAsh, but simplify it and move code from it that is not c/b/ui specific out of it (and replace the TODO comment with a "c/b/ui/" code only comment). Thoughts? (I will wait until jamescook@ returns to move forward on this)
,
Jan 2 2018
,
Jan 2 2018
I filed 798569 with my ideas as to what should be in the directories. Once that is done it should be easier to have the ui specific startup parts live in c/b/ui/chromeos. If it's too hard to untangle all the UI specific chromeos startup parts, then we could move all of ChromeBrowserMainPartsChromeOS into c/b/ui/chromeos.
,
Jan 2 2018
I don't especially like the idea of moving ChromeBrowserMainPartsChromeOS to c/b/ui, but I definitely agree that we should untangle c/b/chromeos and c/b/ui/ash.
,
Jan 3 2018
I commented on issue 798569 re: overall directory structure. I think getting rid of AshInit, or at least moving it into one of the other ChromeBrowserMainExtraParts files, isn't controversial. It would be nice to move it into the final "correct" place, though.
,
Jan 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43b3c210187b733e95d2d185d4a3467ad8092c83 commit 43b3c210187b733e95d2d185d4a3467ad8092c83 Author: Steven Bennetts <stevenjb@chromium.org> Date: Mon Jan 08 17:33:25 2018 Move keyboard init to ChromeBrowserMainChromeOS ChromeBrowserMainPartsAsh should only contain initialization that depends on c/b/ui/ash. This also moves all calls to RootWindowController::ActivateKeyboard to ash::Shell. Bug: 796007 Change-Id: I22704c765e1fa17b16b29aa995509d6e7cf44052 Reviewed-on: https://chromium-review.googlesource.com/851134 Commit-Queue: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#527662} [modify] https://crrev.com/43b3c210187b733e95d2d185d4a3467ad8092c83/ash/shell.cc [modify] https://crrev.com/43b3c210187b733e95d2d185d4a3467ad8092c83/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/43b3c210187b733e95d2d185d4a3467ad8092c83/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc [modify] https://crrev.com/43b3c210187b733e95d2d185d4a3467ad8092c83/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.h
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33 commit 6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33 Author: Steven Bennetts <stevenjb@chromium.org> Date: Tue Jan 09 18:07:32 2018 Move non Shell code from AshInit and rename AshShellInit Bug: 796007 Change-Id: I61b3022da24b17a481ea90e1cbc17786bd12686f Reviewed-on: https://chromium-review.googlesource.com/852366 Commit-Queue: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#528042} [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/display/cursor_window_controller_unittest.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/extended_desktop_unittest.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/shell.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/shell.h [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/wm/window_manager_unittest.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/ash/wm/workspace/workspace_window_resizer_unittest.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/chromeos/login/login_browsertest.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/chromeos/login/session/chrome_session_manager.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/ui/BUILD.gn [delete] https://crrev.com/399e397c9f28f0a176a3ef11b3a660be54a85505/chrome/browser/ui/ash/ash_init.h [rename] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/ui/ash/ash_shell_init.cc [add] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/ui/ash/ash_shell_init.h [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc [modify] https://crrev.com/6d12eb46a70040adc6c5087c7f2e0dc4ed1cce33/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9301536fce9e29bb9a28ec86366eae1ef11f0dc commit f9301536fce9e29bb9a28ec86366eae1ef11f0dc Author: Steven Bennetts <stevenjb@chromium.org> Date: Wed Jan 10 19:34:15 2018 Move remaining UI code to ChromeBrowserMainExtraPartsAsh This removes the remaining UI code from ChromeBrowserMainPartsChromeOS and moves it to ChromeBrowserMainExtraPartsAsh. This also: * Replaces instances of the deprecated ash_util::IsRunningInMash() with chromeos::GetAshConfig() == ash::Config::MASH. * Alphabetizes construction of instances in ChromeBrowserMainExtraPartsAsh (they should not have any implicit co-dependencies). * Moves PushProcessCreationTimeToAsh() to ChromeBrowserMainExtraPartsAsh. Bug: 796007 For DEPS changes: TBR=oshima@chromium.org Change-Id: I8a1d45d3ef12ee5f31781e774e42dc4098faa5d6 Reviewed-on: https://chromium-review.googlesource.com/857322 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Commit-Queue: Steven Bennetts <stevenjb@chromium.org> Cr-Commit-Position: refs/heads/master@{#528381} [modify] https://crrev.com/f9301536fce9e29bb9a28ec86366eae1ef11f0dc/chrome/browser/chromeos/DEPS [modify] https://crrev.com/f9301536fce9e29bb9a28ec86366eae1ef11f0dc/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/f9301536fce9e29bb9a28ec86366eae1ef11f0dc/chrome/browser/chromeos/chrome_browser_main_chromeos.h [modify] https://crrev.com/f9301536fce9e29bb9a28ec86366eae1ef11f0dc/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc [modify] https://crrev.com/f9301536fce9e29bb9a28ec86366eae1ef11f0dc/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h
,
Jan 11 2018
Updated the title to reflect the work: * All non-ash related chromeos initialization has been moved to ChromeBrowserMainPartsChromeOS * Ash specific initialization has been moved to ChromeBrowserMainExtraPartsAsh * AshInit has been renamed AshShellInit and is only responsible for initializing the Ash Shell.
,
Jan 11 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by steve...@chromium.org
, Dec 19 2017