Consolidate common logic in DefaultBrowserSurfaceManager [chromium/src : main]
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Foromo Daniel Soromou
| Commit-Queue | +1 |
1 comment
// tabs, so we don't need to do anything here for new browsers.This comment is slightly misleading. `Init()` handles browsers that exist when `Show()` is called. For browsers created after that, the `BrowserTabStripTracker` will observe them and add infobars as needed.
Suggestion:
```cpp
// The BrowserTabStripTracker will handle showing infobars for both existing
// and newly created browsers, so we don't need to do anything here.
```
Related details
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 8 comments![]( "Open in Gerrit")
bool DefaultBrowserInfoBarManager::ShouldTrackBrowser(could we replace this with DefaultBrowserSurfaceManager::IsBrowserValidForShowing
// BrowserCollectionObserver:
void OnBrowserCreated(BrowserWindowInterface* browser) override;
void OnBrowserClosed(BrowserWindowInterface* browser) override;These should be public, since we're using public inheritance.
bool can_pin_to_taskbar_ = false;Let's make this private and expose a getter method instead, so that child classes cannot modify this. We can consider adding a setter later if needed. Same for `controller_`.
virtual void CloseAllUI() = 0;supernit: "UI" sounds ambiguous: the first question that comes to mind is if this includes all UI for default browser, or just this surface. Perhaps `CloseAllSurfaceInstances` or something like that?
virtual void ShowUIForBrowser(BrowserWindowInterface* browser) = 0;supernit: Show/CloseForBrowser seems sufficient
protected:Are these methods called by any of the derived classes? If not, let's make them private
std::unique_ptr<default_browser::DefaultBrowserController> controller,Could we instead create the controller within this method and destroy it in another method from this class so that the lifecycle is completely encapsulated?
controller_->OnShown();It might be a bit confusing that only a subset of actions are forwarded to the controller, which can skew data if a derived class forgets about the controller.
I think it would be cleaner/more consistent if either this class handles 100% of the interactions with controller (allowing us to hide it from derived classes), or handles none of the interactions. I have a slight preference for the first option, which would behave nicely with my previous comment.
Related details
- Foromo Daniel Soromou
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 8 comments![]( "Open in Gerrit")
could we replace this with DefaultBrowserSurfaceManager::IsBrowserValidForShowing
Done
// BrowserCollectionObserver:
void OnBrowserCreated(BrowserWindowInterface* browser) override;
void OnBrowserClosed(BrowserWindowInterface* browser) override;These should be public, since we're using public inheritance.
Done
Let's make this private and expose a getter method instead, so that child classes cannot modify this. We can consider adding a setter later if needed. Same for `controller_`.
Done
supernit: "UI" sounds ambiguous: the first question that comes to mind is if this includes all UI for default browser, or just this surface. Perhaps `CloseAllSurfaceInstances` or something like that?
Done
virtual void ShowUIForBrowser(BrowserWindowInterface* browser) = 0;Foromo Daniel Soromousupernit: Show/CloseForBrowser seems sufficient
Done
protected:Are these methods called by any of the derived classes? If not, let's make them private
They are implemented by the derived classes. That is why it should be at least protected since derived classes cannot implement private methods from parent class.
std::unique_ptr<default_browser::DefaultBrowserController> controller,Could we instead create the controller within this method and destroy it in another method from this class so that the lifecycle is completely encapsulated?
Done
It might be a bit confusing that only a subset of actions are forwarded to the controller, which can skew data if a derived class forgets about the controller.
I think it would be cleaner/more consistent if either this class handles 100% of the interactions with controller (allowing us to hide it from derived classes), or handles none of the interactions. I have a slight preference for the first option, which would behave nicely with my previous comment.
- Kaan Alsan
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. |
Kaan Alsan (Gerrit)
Kaan Alsan voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kaan Alsan
| Code-Review | +1 |
2 comments
lgtm % making the pure virtual methods private
protected:Foromo Daniel SoromouAre these methods called by any of the derived classes? If not, let's make them private
They are implemented by the derived classes. That is why it should be at least protected since derived classes cannot implement private methods from parent class.
Child classes can implement private methods from the parent class. By making it private, we're essentially saying "the child class should implement this, but only the base class will invoke it".
Related details
- Foromo Daniel Soromou
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. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
protected:Foromo Daniel SoromouAre these methods called by any of the derived classes? If not, let's make them private
Kaan AlsanThey are implemented by the derived classes. That is why it should be at least protected since derived classes cannot implement private methods from parent class.
Child classes can implement private methods from the parent class. By making it private, we're essentially saying "the child class should implement this, but only the base class will invoke it".
Oh got it, Thank you for the clarification.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou 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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/startup/default_browser_prompt/default_browser_surface_manager.h
Insertions: 10, Deletions: 10.
@@ -49,6 +49,16 @@
bool can_pin_to_taskbar() const { return can_pin_to_taskbar_; }
protected:
+ // Helper function to determine if a browser window is suitable for showing a
+ // prompt. Excludes incognito, guest, and non-normal browser windows.
+ bool IsBrowserValidForShowing(BrowserWindowInterface* browser);
+
+ // Methods for derived classes to handle user interactions via the controller.
+ void HandleAccept();
+ void HandleDismiss();
+ void HandleIgnore();
+
+ private:
// Abstract methods to be implemented by subclasses to handle UI operations.
//
// Shows the specific prompt UI for the given browser window.
@@ -60,16 +70,6 @@
// Closes all prompt instances managed by the subclass.
virtual void CloseAllPromptInstances() = 0;
- // Helper function to determine if a browser window is suitable for showing a
- // prompt. Excludes incognito, guest, and non-normal browser windows.
- bool IsBrowserValidForShowing(BrowserWindowInterface* browser);
-
- // Methods for derived classes to handle user interactions via the controller.
- void HandleAccept();
- void HandleDismiss();
- void HandleIgnore();
-
- private:
// Flag indicating if the taskbar pinning option should be available.
bool can_pin_to_taskbar_ = false;
```
Change information
Consolidate common logic in DefaultBrowserSurfaceManager
This refactoring transforms `DefaultBrowserSurfaceManager` from an
interface into an abstract base class to handle shared functionality for
different default browser prompt types.
Previously, `DefaultBrowserBubbleDialogManager` and
`DefaultBrowserInfoBarManager` duplicated logic for observing browser
window creation and destruction, managing the
`DefaultBrowserController`, and tracking the `can_pin_to_taskbar` state.
With the upcoming `DefaultBrowserModalDialogManager` more code will be
duplicated.
This change centralizes that logic within the base class by:
* Moving `BrowserCollectionObserver` inheritance and related lifecycle
management to `DefaultBrowserSurfaceManager`.
* Providing a shared `IsBrowserValidForShowing` helper to exclude
incognito, guest, and non-normal browser windows consistently.
* Introducing abstract methods (`ShowUIForBrowser`,
`CloseUIForBrowser`, and `CloseAllUI`) that subclasses must
implement for their specific UI presentation logic.
Consolidating this logic reduces code duplication and simplifies the
concrete implementations, ensuring a unified approach to displaying
default browser prompts across the application.
- M chrome/browser/ui/startup/default_browser_prompt/BUILD.gn
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_bubble_dialog_interactive_uitest.cc
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_bubble_dialog_manager.cc
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_bubble_dialog_manager.h
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_infobar_manager.cc
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_infobar_manager.h
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_prompt_manager.cc
- A chrome/browser/ui/startup/default_browser_prompt/default_browser_surface_manager.cc
- M chrome/browser/ui/startup/default_browser_prompt/default_browser_surface_manager.h
Code-Review: +1 by Kaan Alsan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |