DevTools/Android: Honor browserContextId in createTarget [chromium/src : main]
Dmitry Gozman (Gerrit)
Dmitry Gozman added 3 comments![]( "Open in Gerrit")
DestroyOTRProfileWhenAppropriate(profile->GetWeakPtr());Since we are no longer waiting for all tabs to be closed, does that mean `CloseTabAt()` does the job synchronously?
GURL gurl = NormalizeTargetUrl(url);I wonder why extract a method? Not that I am really against it, but seems unnecessary 😊
DevToolsManagerDelegateAndroid::MarkCreatedByDevTools(*web_contents);
if (for_tab.value_or(false)) {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateForTab(web_contents)->GetId();
} else {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateFor(web_contents)->GetId();
}
return protocol::Response::Success();
}Can we reuse this code between browser_context_id vs default branches?
Related details
- Alex Rudenko
- Julio Piubello
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 2 comments![]( "Open in Gerrit")
std::move(callback).Run(true, "");Since you updated `DisposeBrowserContext` to use `TabModelList` on Android because `BrowserWindowInterface` doesn't work for Android tabs, shouldn't `OnProfileWillBeDestroyed` be updated as well?
If `OnProfileWillBeDestroyed` continues to use `ForEachCurrentBrowserWindowInterfaceOrderedByActivation`, it won't close tabs with the destroyed profile on Android, which could lead to a use-after-free when the profile is destroyed?
WebContents* web_contents = CreateTabInBrowserContext(profile, gurl);The logic for marking the tab and getting the target ID here is identical to the fallback case on lines 170-186. You can deduplicate this by assigning `web_contents` in an `if/else` block and then running the common logic once.
```cpp
WebContents* web_contents = nullptr;
if (browser_context_id.has_value()) {
Profile* profile =
DevToolsBrowserContextManager::GetInstance().GetProfileById(
*browser_context_id);
if (!profile) {
return protocol::Response::ServerError(
"Failed to find browser context with id " + *browser_context_id);
}
web_contents = CreateTabInBrowserContext(profile, gurl);
} else {
web_contents =
tab_model->CreateNewTabForDevTools(gurl, new_window.value_or(false));
}
if (!web_contents) {
return protocol::Response::ServerError("Could not create a Tab");
}DevToolsManagerDelegateAndroid::MarkCreatedByDevTools(*web_contents);
if (for_tab.value_or(false)) {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateForTab(web_contents)->GetId();
} else {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateFor(web_contents)->GetId();
}- Julio Piubello
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. |
Julio Piubello (Gerrit)
Julio Piubello added 5 comments![]( "Open in Gerrit")
DestroyOTRProfileWhenAppropriate(profile->GetWeakPtr());Since we are no longer waiting for all tabs to be closed, does that mean `CloseTabAt()` does the job synchronously?
yes, that path does not enqueue a pending closure, and TabCollectionTabModelImpl.closeTabsInternal() removes and finalizes the tab before returning.
do you think I should add a small comment?
Since you updated `DisposeBrowserContext` to use `TabModelList` on Android because `BrowserWindowInterface` doesn't work for Android tabs, shouldn't `OnProfileWillBeDestroyed` be updated as well?
If `OnProfileWillBeDestroyed` continues to use `ForEachCurrentBrowserWindowInterfaceOrderedByActivation`, it won't close tabs with the destroyed profile on Android, which could lead to a use-after-free when the profile is destroyed?
that makes sense, thanks. changed to use the same Android TabModelList-based cleanup
I wonder why extract a method? Not that I am really against it, but seems unnecessary 😊
removed!
WebContents* web_contents = CreateTabInBrowserContext(profile, gurl);The logic for marking the tab and getting the target ID here is identical to the fallback case on lines 170-186. You can deduplicate this by assigning `web_contents` in an `if/else` block and then running the common logic once.
```cpp
WebContents* web_contents = nullptr;
if (browser_context_id.has_value()) {
Profile* profile =
DevToolsBrowserContextManager::GetInstance().GetProfileById(
*browser_context_id);
if (!profile) {
return protocol::Response::ServerError(
"Failed to find browser context with id " + *browser_context_id);
}
web_contents = CreateTabInBrowserContext(profile, gurl);
} else {
web_contents =
tab_model->CreateNewTabForDevTools(gurl, new_window.value_or(false));
}if (!web_contents) {
return protocol::Response::ServerError("Could not create a Tab");
}DevToolsManagerDelegateAndroid::MarkCreatedByDevTools(*web_contents);if (for_tab.value_or(false)) {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateForTab(web_contents)->GetId();
} else {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateFor(web_contents)->GetId();
}return protocol::Response::Success();
```
sure thing, done
DevToolsManagerDelegateAndroid::MarkCreatedByDevTools(*web_contents);
if (for_tab.value_or(false)) {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateForTab(web_contents)->GetId();
} else {
*out_target_id =
content::DevToolsAgentHost::GetOrCreateFor(web_contents)->GetId();
}
return protocol::Response::Success();
}Can we reuse this code between browser_context_id vs default branches?
- Alex Rudenko
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |