[Ash] Separate VideoConferenceManagerAsh from CrosapiAsh [chromium/src : main]
Di Wu (Gerrit)
Di Wu added 2 comments![]( "Open in Gerrit")
video_conference_manager_ash_ =@hide...@chromium.org @ne...@chromium.org @khor...@chromium.org The timing of initializing this globally accessed `video_conference_manager_ash_` is a bit tricky as I came to realize.
As it turns out, there are at least four *clients* that rely upon the successful initialization of `video_conference_manager_ash_` for themselves to properly function. They are:
1.`video_conference_manager_client_`
2.`vc_app_service_client_`
3.`vc_ash_feature_client_`
4.`chrome_capture_mode_delegate`
Please note that the construction of clients 1, 2, 3 mentioned above are in the same file that we are looking at right now, i.e., `//c/b/a/main_parts/chrome_browser_main_parts_ash`. So naturally if we can initialize `video_conference_manager_ash_` at the same time when clients 1, 2, 3 are initialized, that would be good. And we may even go one step further, using depedency injection (passing pointer of `video_conference_manager_ash_` to the said clients' ctors) instead of accessing through singletons.
This would require we push the initializing timing from `PreProfileInit()` to `PostBrowserStart()`.
Sadly if we push it here, the timing would be too late for the fourth client, i.e., `chrome_capture_mode_delegate` to access `video_conference_manager_ash_`, which results in immediate crash for tens of browser tests. And here's one more thing that makes matters worse, `chrome_capture_mode_delegate` is initialized in a separate context, in ash's `Shell::Init()`. So coordinating the initialization timing of these four clients is practically very difficult if not impossible.
To sum up, the proposed initialization timing for `video_conference_manager_ash_`, as is shown in this CL, in `PreProfileInit()` right after the initialization of the original `crosapi_manager_`, seems for now the only feasible option.
class VideoConferenceManagerAsh : public VideoConferenceManagerBase {@hide...@chromium.org @ne...@chromium.org @khor...@chromium.org
Quote from commit message:
This CL also doesn't try to remove the now redundant
`VideoConferenceManagerBase` interface class. Yes this class is made
redundant due to Lacros sunset. Sadly this interface class also bears
the burden of preventing
`//ash/system/video_conference/video_conference_tray_controller.cc` from
having to depend on
`//chrome/browser/ash/video_conference/video_conference_manager_ash.h`.
If, there's another way to make this happen, that is, deleting `VideoConferenceManagerBase` and related code without making the current depedency structure worse, I'm all ears.
Related details
- Georg Neis
- Hidehiko Abe
- Kyle Horimoto
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis added 2 comments![]( "Open in Gerrit")
if (features::IsVideoConferenceEnabled()) {Are you sure this change (making the construction conditional) is safe? I'd leave it out of the CL. If it's safe then there is other related cleanup possible (see the check inside the constructor and several TODOs), so a separate CL would make sense.
video_conference_manager_ash_.reset();In principle this should happen before browser_manager_.reset() (reverse order of construction). Also looks like we forgot to reset magic_boost_controller_ash_.
Related details
- Di Wu
- Hidehiko Abe
- Kyle Horimoto
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 2 comments![]( "Open in Gerrit")
Are you sure this change (making the construction conditional) is safe? I'd leave it out of the CL. If it's safe then there is other related cleanup possible (see the check inside the constructor and several TODOs), so a separate CL would make sense.
Good call. While I'm getting surer that this change is safe as this patch got through CQ. I also agree this flag check needs to be handled in a speparate CL. After all as ash refactorers our work is supposed to be transparent (invisible) when it comes to product logic. So I dropped this flag check for the sake of this CL.
In principle this should happen before browser_manager_.reset() (reverse order of construction). Also looks like we forgot to reset magic_boost_controller_ash_.
Agreed and agreed. In the latest patch you will find the `reset()` here moved to earlier, right before `browser_manager_.reset()`. Also, in a sperate CL let me add back the reset for `magic_boost_controller_ash_`.
Related details
- Georg Neis
- Hidehiko Abe
- Kyle Horimoto
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis voted Code-Review+1![]( "Open in Gerrit")
- Di Wu
- Hidehiko Abe
- Kyle Horimoto
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 1 comment![]( "Open in Gerrit")
video_conference_manager_ash_ =@hide...@chromium.org @ne...@chromium.org @khor...@chromium.org The timing of initializing this globally accessed `video_conference_manager_ash_` is a bit tricky as I came to realize.
As it turns out, there are at least four *clients* that rely upon the successful initialization of `video_conference_manager_ash_` for themselves to properly function. They are:
1.`video_conference_manager_client_`
2.`vc_app_service_client_`
3.`vc_ash_feature_client_`
4.`chrome_capture_mode_delegate`Please note that the construction of clients 1, 2, 3 mentioned above are in the same file that we are looking at right now, i.e., `//c/b/a/main_parts/chrome_browser_main_parts_ash`. So naturally if we can initialize `video_conference_manager_ash_` at the same time when clients 1, 2, 3 are initialized, that would be good. And we may even go one step further, using depedency injection (passing pointer of `video_conference_manager_ash_` to the said clients' ctors) instead of accessing through singletons.
This would require we push the initializing timing from `PreProfileInit()` to `PostBrowserStart()`.
Sadly if we push it here, the timing would be too late for the fourth client, i.e., `chrome_capture_mode_delegate` to access `video_conference_manager_ash_`, which results in immediate crash for tens of browser tests. And here's one more thing that makes matters worse, `chrome_capture_mode_delegate` is initialized in a separate context, in ash's `Shell::Init()`. So coordinating the initialization timing of these four clients is practically very difficult if not impossible.
To sum up, the proposed initialization timing for `video_conference_manager_ash_`, as is shown in this CL, in `PreProfileInit()` right after the initialization of the original `crosapi_manager_`, seems for now the only feasible option.
Thank you for investigation. I wasn't aware ChromeCaptureModeDelegate.
Then, could you pass the ptr of VideoConferenceManagerAsh's ptr to each initialization?
For 1.-3., you can access video_conference_manager_ash_ directly on construction, because both are in the same instance.
For 4., you can pass the ptr to ChromeShellDelegate via
chrome/browser/ui/ash/main_extra_parts/chrome_browser_main_extra_parts_ash.cc
Also, I'm wondering if the 1.-3. and video_conference_manager_ash_ can be moved to ChromeBrowserMainExtraPartsAsh, so the initialization can be much simplified.
Related details
- Di Wu
- Kyle Horimoto
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 1 comment![]( "Open in Gerrit")
video_conference_manager_ash_ =Hidehiko Abe@hide...@chromium.org @ne...@chromium.org @khor...@chromium.org The timing of initializing this globally accessed `video_conference_manager_ash_` is a bit tricky as I came to realize.
As it turns out, there are at least four *clients* that rely upon the successful initialization of `video_conference_manager_ash_` for themselves to properly function. They are:
1.`video_conference_manager_client_`
2.`vc_app_service_client_`
3.`vc_ash_feature_client_`
4.`chrome_capture_mode_delegate`Please note that the construction of clients 1, 2, 3 mentioned above are in the same file that we are looking at right now, i.e., `//c/b/a/main_parts/chrome_browser_main_parts_ash`. So naturally if we can initialize `video_conference_manager_ash_` at the same time when clients 1, 2, 3 are initialized, that would be good. And we may even go one step further, using depedency injection (passing pointer of `video_conference_manager_ash_` to the said clients' ctors) instead of accessing through singletons.
This would require we push the initializing timing from `PreProfileInit()` to `PostBrowserStart()`.
Sadly if we push it here, the timing would be too late for the fourth client, i.e., `chrome_capture_mode_delegate` to access `video_conference_manager_ash_`, which results in immediate crash for tens of browser tests. And here's one more thing that makes matters worse, `chrome_capture_mode_delegate` is initialized in a separate context, in ash's `Shell::Init()`. So coordinating the initialization timing of these four clients is practically very difficult if not impossible.
To sum up, the proposed initialization timing for `video_conference_manager_ash_`, as is shown in this CL, in `PreProfileInit()` right after the initialization of the original `crosapi_manager_`, seems for now the only feasible option.
Thank you for investigation. I wasn't aware ChromeCaptureModeDelegate.
Then, could you pass the ptr of VideoConferenceManagerAsh's ptr to each initialization?
For 1.-3., you can access video_conference_manager_ash_ directly on construction, because both are in the same instance.
For 4., you can pass the ptr to ChromeShellDelegate via
chrome/browser/ui/ash/main_extra_parts/chrome_browser_main_extra_parts_ash.ccAlso, I'm wondering if the 1.-3. and video_conference_manager_ash_ can be moved to ChromeBrowserMainExtraPartsAsh, so the initialization can be much simplified.
@hide...@chromium.org Thanks for the wonderful guidance. Agreed on every bit of it.
For 1-3 (and maybe for 4 too), right now I am thinking about utilizing a `weak_ptr` as the storage within each client. And I drafted a [demo CL] to showcase my plan. Can you take a look and let me know what you think? A second plan would be using `shared_ptr` to manage a reference counted object.
[demo CL]:https://chromium-review.googlesource.com/c/chromium/src/+/6235246
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 1 comment![]( "Open in Gerrit")
video_conference_manager_ash_ =Hidehiko Abe@hide...@chromium.org @ne...@chromium.org @khor...@chromium.org The timing of initializing this globally accessed `video_conference_manager_ash_` is a bit tricky as I came to realize.
As it turns out, there are at least four *clients* that rely upon the successful initialization of `video_conference_manager_ash_` for themselves to properly function. They are:
1.`video_conference_manager_client_`
2.`vc_app_service_client_`
3.`vc_ash_feature_client_`
4.`chrome_capture_mode_delegate`Please note that the construction of clients 1, 2, 3 mentioned above are in the same file that we are looking at right now, i.e., `//c/b/a/main_parts/chrome_browser_main_parts_ash`. So naturally if we can initialize `video_conference_manager_ash_` at the same time when clients 1, 2, 3 are initialized, that would be good. And we may even go one step further, using depedency injection (passing pointer of `video_conference_manager_ash_` to the said clients' ctors) instead of accessing through singletons.
This would require we push the initializing timing from `PreProfileInit()` to `PostBrowserStart()`.
Sadly if we push it here, the timing would be too late for the fourth client, i.e., `chrome_capture_mode_delegate` to access `video_conference_manager_ash_`, which results in immediate crash for tens of browser tests. And here's one more thing that makes matters worse, `chrome_capture_mode_delegate` is initialized in a separate context, in ash's `Shell::Init()`. So coordinating the initialization timing of these four clients is practically very difficult if not impossible.
To sum up, the proposed initialization timing for `video_conference_manager_ash_`, as is shown in this CL, in `PreProfileInit()` right after the initialization of the original `crosapi_manager_`, seems for now the only feasible option.
Di WuThank you for investigation. I wasn't aware ChromeCaptureModeDelegate.
Then, could you pass the ptr of VideoConferenceManagerAsh's ptr to each initialization?
For 1.-3., you can access video_conference_manager_ash_ directly on construction, because both are in the same instance.
For 4., you can pass the ptr to ChromeShellDelegate via
chrome/browser/ui/ash/main_extra_parts/chrome_browser_main_extra_parts_ash.ccAlso, I'm wondering if the 1.-3. and video_conference_manager_ash_ can be moved to ChromeBrowserMainExtraPartsAsh, so the initialization can be much simplified.
@hide...@chromium.org Thanks for the wonderful guidance. Agreed on every bit of it.
For 1-3 (and maybe for 4 too), right now I am thinking about utilizing a `weak_ptr` as the storage within each client. And I drafted a [demo CL] to showcase my plan. Can you take a look and let me know what you think? A second plan would be using `shared_ptr` to manage a reference counted object.
[demo CL]:https://chromium-review.googlesource.com/c/chromium/src/+/6235246
Because we should know the lifetime of global objects, I think we should use raw_ptr, rather than weak_ptr or shared_ptr.
Related details
- Di Wu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Wu (Gerrit)
Di Wu added 1 comment![]( "Open in Gerrit")
Following your very insightful advice in this thread, I drafted a follow-up CL that tried to specifically get rid of the current singleton design and to replace it with dependency injection either through ctor or through a dedicated setter if ctor is not possible. Please take a look and let me know if you have any questions. Many thanks.
https://chromium-review.googlesource.com/c/chromium/src/+/6256260/1
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. |
Di Wu (Gerrit)
Di Wu abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |