Add a multi-modal glic sharing manager. [chromium/src : main]
Bryant Chandler (Gerrit)
Bryant Chandler added 2 comments![]( "Open in Gerrit")
class GlicMultiModalSharingManager {I wonder if there's a different name we could use for this. I was a little confused with calling it a sharing manager since it doesn't implement the sharing manager interface.
std::make_unique<GlicMultiModalSharingManager>(profile,Does this actually need to be a unique_ptr?
Related details
- Marke Hallowell
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marke Hallowell (Gerrit)
Marke Hallowell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marke Hallowell
| Commit-Queue | +1 |
2 comments
class GlicMultiModalSharingManager {I wonder if there's a different name we could use for this. I was a little confused with calling it a sharing manager since it doesn't implement the sharing manager interface.
Done
std::make_unique<GlicMultiModalSharingManager>(profile,Does this actually need to be a unique_ptr?
- Bryant Chandler
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bryant Chandler (Gerrit)
Bryant Chandler added 2 comments![]( "Open in Gerrit")
#if !BUILDFLAG(IS_ANDROID)
std::make_unique<GlicPinnedTabManagerImpl>(profile,
ui_delegate,
metrics)
#else
std::make_unique<GlicEmptyPinnedTabManager>()
#endifThe real pinned tab manager now compiles on android, so we can get rid of this ifdef
sharing_manager_coordinator_.OnGlicWindowActivationChanged(is_active &&Seems like we could use sharing_manager() and not add this method to the coordinator at all. It's just delegating anyway.
Related details
- Marke Hallowell
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marke Hallowell (Gerrit)
Marke Hallowell added 2 comments![]( "Open in Gerrit")
#if !BUILDFLAG(IS_ANDROID)
std::make_unique<GlicPinnedTabManagerImpl>(profile,
ui_delegate,
metrics)
#else
std::make_unique<GlicEmptyPinnedTabManager>()
#endifThe real pinned tab manager now compiles on android, so we can get rid of this ifdef
Done
sharing_manager_coordinator_.OnGlicWindowActivationChanged(is_active &&Seems like we could use sharing_manager() and not add this method to the coordinator at all. It's just delegating anyway.
I prefer keeping "sharing_manager()" limited to the broader sharing manager interface (which this method is not currently part of). So we could return the very specific sharing manager instance here and have it use that, but that limits what the coordinator is allowed to do internally (I think we want the layer of indirection to be a layer of indirection and not leak implementation details).
This seemed liked the best short term option to me, but if you feel strongly about it I can refactor it to return a GlicStablePinningSharingManager instead and then use it directly.
Related details
- Bryant Chandler
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bryant Chandler (Gerrit)
Bryant Chandler voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bryant Chandler
| Code-Review | +1 |
1 comment
sharing_manager_coordinator_.OnGlicWindowActivationChanged(is_active &&Marke HallowellSeems like we could use sharing_manager() and not add this method to the coordinator at all. It's just delegating anyway.
I prefer keeping "sharing_manager()" limited to the broader sharing manager interface (which this method is not currently part of). So we could return the very specific sharing manager instance here and have it use that, but that limits what the coordinator is allowed to do internally (I think we want the layer of indirection to be a layer of indirection and not leak implementation details).
This seemed liked the best short term option to me, but if you feel strongly about it I can refactor it to return a GlicStablePinningSharingManager instead and then use it directly.
Ah. I had missed that. If we want to keep it in the private interface then we can leave it as is.
Related details
- Marke Hallowell
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marke Hallowell (Gerrit)
Marke Hallowell voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Add a multi-modal glic sharing manager.
GlicInstanceImpl requires mode-specific sharing manager implementations.
This cl moves the management of that out of GlicInstanceImpl itself and
into a newly created GlicMultiModalSharingManager class.
- M chrome/browser/glic/BUILD.gn
- A chrome/browser/glic/host/context/glic_sharing_manager_coordinator.cc
- A chrome/browser/glic/host/context/glic_sharing_manager_coordinator.h
- M chrome/browser/glic/service/glic_instance_impl.cc
- M chrome/browser/glic/service/glic_instance_impl.h
Code-Review: +1 by Bryant Chandler
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |