FindTabbedBrowser() function call point refactoringYu Henit: Please briefly explain why and what was done.
fixed
#include "chrome/browser/ui/browser_window/public/browser_window_interface.h"Yu Henit: Please use forward declarations whenever possible.
fixed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return browser->GetBrowserForMigrationOnly();nit: We can try to avoid use GetBrowserForMigrationOnly() by change return type of this lambda. Thanks~
target_browser->GetBrowserForMigrationOnly());```
void ReparentWebContentsIntoBrowserImpl(Browser* source_browser,
content::WebContents* web_contents,
BrowserWindowInterface* target_browser,
bool insert_as_pinned_home_tab) {
```
nit: Seems do not need to use the first target_browser->GetBrowserForMigrationOnly() here.
if (!browser_) {
return nullptr;
}nit: Is this necessary?
: existing_browser_window->GetBrowserForMigrationOnly();nit: target_browser_window can simply migrate to BrowserWindowInterface* here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return browser->GetBrowserForMigrationOnly();nit: We can try to avoid use GetBrowserForMigrationOnly() by change return type of this lambda. Thanks~
fixed
target_browser->GetBrowserForMigrationOnly());```
void ReparentWebContentsIntoBrowserImpl(Browser* source_browser,
content::WebContents* web_contents,
BrowserWindowInterface* target_browser,
bool insert_as_pinned_home_tab) {
```
nit: Seems do not need to use the first target_browser->GetBrowserForMigrationOnly() here.
agree
if (!browser_) {
return nullptr;
}nit: Is this necessary?
Yes, previously, this function return 'browser_' and the called function it will check browser pointer, but now return type became 'browser_->GetBrowserForMigrationOnly()', we should check 'browser_' pointer, here `browser_` is dereferenced.
: existing_browser_window->GetBrowserForMigrationOnly();nit: target_browser_window can simply migrate to BrowserWindowInterface* here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return browser->GetBrowserForMigrationOnly();Yu Henit: We can try to avoid use GetBrowserForMigrationOnly() by change return type of this lambda. Thanks~
fixed
I think we can't avoid GetBrowserForMigrationOnly().
A lambda expression returns a BrowserWindowInterface*, while a FunctionRef requires a Browser*. A BrowserWindowInterface* (the parent class) cannot be implicitly cast to a Browser* (the subclass).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gfx::NativeWindow GetUninstallAnchorWindow() const override {
return chrome::FindTabbedBrowser(profile_, false)
->GetWindow()
->GetNativeWindow();
}nit: FindTabbedBrowser may return nullptr, while we’re here, we can add an extra layer of protection.
```
gfx::NativeWindow GetUninstallAnchorWindow() const override {
BrowserWindowInterface* browser =
chrome::FindTabbedBrowser(profile_, false);
return browser ? browser->GetWindow()->GetNativeWindow() : nullptr;
}
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gfx::NativeWindow GetUninstallAnchorWindow() const override {
return chrome::FindTabbedBrowser(profile_, false)
->GetWindow()
->GetNativeWindow();
}nit: FindTabbedBrowser may return nullptr, while we’re here, we can add an extra layer of protection.
```
gfx::NativeWindow GetUninstallAnchorWindow() const override {
BrowserWindowInterface* browser =
chrome::FindTabbedBrowser(profile_, false);
return browser ? browser->GetWindow()->GetNativeWindow() : nullptr;
}
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
target_browser->GetBrowserForMigrationOnly());Yu He```
void ReparentWebContentsIntoBrowserImpl(Browser* source_browser,
content::WebContents* web_contents,
BrowserWindowInterface* target_browser,
bool insert_as_pinned_home_tab) {
```
nit: Seems do not need to use the first target_browser->GetBrowserForMigrationOnly() here.
agree
Done
if (!browser_) {
return nullptr;
}Yu Henit: Is this necessary?
Yes, previously, this function return 'browser_' and the called function it will check browser pointer, but now return type became 'browser_->GetBrowserForMigrationOnly()', we should check 'browser_' pointer, here `browser_` is dereferenced.
Done
: existing_browser_window->GetBrowserForMigrationOnly();Yu Henit: target_browser_window can simply migrate to BrowserWindowInterface* here.
fixed.
Done
gfx::NativeWindow GetUninstallAnchorWindow() const override {
return chrome::FindTabbedBrowser(profile_, false)
->GetWindow()
->GetNativeWindow();
}Yu Henit: FindTabbedBrowser may return nullptr, while we’re here, we can add an extra layer of protection.
```
gfx::NativeWindow GetUninstallAnchorWindow() const override {
BrowserWindowInterface* browser =
chrome::FindTabbedBrowser(profile_, false);
return browser ? browser->GetWindow()->GetNativeWindow() : nullptr;
}
```
fixed
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
return browser->GetBrowserForMigrationOnly();Yu Henit: We can try to avoid use GetBrowserForMigrationOnly() by change return type of this lambda. Thanks~
Yu Hefixed
I think we can't avoid GetBrowserForMigrationOnly().
A lambda expression returns a BrowserWindowInterface*, while a FunctionRef requires a Browser*. A BrowserWindowInterface* (the parent class) cannot be implicitly cast to a Browser* (the subclass).
Yu He
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
| Commit-Queue | +1 |
This CL is ready, please help to review, thanks.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |