CreateBrowserWindow() Accepts a WebContents On Android [chromium/src : main]
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 2 comments![]( "Open in Gerrit")
void attachWebContentsToIntent(This is really creating a new window with a tab holding the specified web contents via a ReparentingTask, which is not clear from the current naming.
My thought is that something like `openWebContentsInNewWindow()` (open to better naming here) might be generally useful. It could look like this:
`openWebContentsInNewWindow(Consumer<Intent> intentDecorator, WebContents webContents, Profile profile, @NewWindowAppSource int source);` to be consistent with our other APIs.
Also on an aside, I'd really like to remove the #createNewWindowIntent() API TBH, since browser_window code is the only external caller currently that invokes this; MultiInstanceOrchestrator should ideally keep the new window intent creation package-private and expose APIs that can internally use this intent (with an optional `Consumer<Intent> intentDecorator` input that callers can use to set additional extras, flags etc.) to create new windows from tabs, links, web contents etc. - +@lin...@google.com for thoughts on this. I know this is a little tricky because of the way browser_window code is currently structured. Could we create a tracking bug to separately explore deprecating and eventually removing the #createNewWindowIntent() API in favor of completely delegating new window creation to the MIO view openNewWindow*() APIs?
Related details
- Jack Thiesen
- Linyu He
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 4 comments![]( "Open in Gerrit")
TabDelegateFactory delegateFactory = CustomTabDelegateFactory.createEmpty();
WindowAndroid window = new WindowAndroid(context, /* occlusionTrackingAllowed= */ false);These will both get replaced during reparenting so no real concern.
android.graphics.Rect bounds = TabUtils.estimateContentSize(context);Properly import?
webContents.setSize(width, height);I think this will get set when the tab is first foregrounded, but we can do this if it helps for some reason?
create_params.web_contents.release());If the creation fails we will leak the object, but I suppose that is somewhat unavoidable.
Related details
- Jack Thiesen
- Linyu He
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 4 comments![]( "Open in Gerrit")
android.graphics.Rect bounds = TabUtils.estimateContentSize(context);Jack ThiesenProperly import?
Done
I think this will get set when the tab is first foregrounded, but we can do this if it helps for some reason?
I copied this from [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java;l=299;drc=1b064913f23efc41aefe42c7a3810da86f857d91) which says:
Resize the webContents to avoid expensive post load resize when attaching the tab.
I took this to mean "this was causing lag/jank as some point, and we triaged it to here".
This is really creating a new window with a tab holding the specified web contents via a ReparentingTask, which is not clear from the current naming.
My thought is that something like `openWebContentsInNewWindow()` (open to better naming here) might be generally useful. It could look like this:
`openWebContentsInNewWindow(Consumer<Intent> intentDecorator, WebContents webContents, Profile profile, @NewWindowAppSource int source);` to be consistent with our other APIs.
Also on an aside, I'd really like to remove the #createNewWindowIntent() API TBH, since browser_window code is the only external caller currently that invokes this; MultiInstanceOrchestrator should ideally keep the new window intent creation package-private and expose APIs that can internally use this intent (with an optional `Consumer<Intent> intentDecorator` input that callers can use to set additional extras, flags etc.) to create new windows from tabs, links, web contents etc. - +@lin...@google.com for thoughts on this. I know this is a little tricky because of the way browser_window code is currently structured. Could we create a tracking bug to separately explore deprecating and eventually removing the #createNewWindowIntent() API in favor of completely delegating new window creation to the MIO view openNewWindow*() APIs?
We agreed offline that the naming of this function is fine.
I've created this bug crbug.com/501427239 to track the second part of this comment and I also added a TODO in `ChromeAndroidTaskTrackerImpl`.
If the creation fails we will leak the object, but I suppose that is somewhat unavoidable.
- Aishwarya Rajesh
- Calder Kitagawa
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
void attachWebContentsToIntent(Jack ThiesenThis is really creating a new window with a tab holding the specified web contents via a ReparentingTask, which is not clear from the current naming.
My thought is that something like `openWebContentsInNewWindow()` (open to better naming here) might be generally useful. It could look like this:
`openWebContentsInNewWindow(Consumer<Intent> intentDecorator, WebContents webContents, Profile profile, @NewWindowAppSource int source);` to be consistent with our other APIs.
Also on an aside, I'd really like to remove the #createNewWindowIntent() API TBH, since browser_window code is the only external caller currently that invokes this; MultiInstanceOrchestrator should ideally keep the new window intent creation package-private and expose APIs that can internally use this intent (with an optional `Consumer<Intent> intentDecorator` input that callers can use to set additional extras, flags etc.) to create new windows from tabs, links, web contents etc. - +@lin...@google.com for thoughts on this. I know this is a little tricky because of the way browser_window code is currently structured. Could we create a tracking bug to separately explore deprecating and eventually removing the #createNewWindowIntent() API in favor of completely delegating new window creation to the MIO view openNewWindow*() APIs?
We agreed offline that the naming of this function is fine.
I've created this bug crbug.com/501427239 to track the second part of this comment and I also added a TODO in `ChromeAndroidTaskTrackerImpl`.
Thanks Jack!
I actually have a WIP CL for this cleanup here: crrev.com/c/7747418.
Sorry for not looking closely at this earlier; I feel like instead of simply doing #setupIntent() in the new MIO API method in this CL, we should invoke #moveTabsToNewWindow() from this API, that should ideally provide the ability to create a new window from a WebContents object end to end vs only setting up the intent - which is the primary problem we're trying to move away from even with #createNewWindowIntent().
I'd imagine this new API to be modeled around the #createNewWindow() API I'm adding in the above CL, but with an additional WebContents input. The MIO should be responsible for both creating the tab / intent as well as starting the activity.
Related details
- Calder Kitagawa
- Jack Thiesen
- Linyu He
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 1 comment![]( "Open in Gerrit")
ReparentingTask.from(tab).setupIntent(intent, null);Use reparenting because this is not obvious whats gonna happen.
Related details
- Aishwarya Rajesh
- Calder Kitagawa
- Linyu He
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 2 comments![]( "Open in Gerrit")
ReparentingTask.from(tab).setupIntent(intent, null);Use reparenting because this is not obvious whats gonna happen.
Obselete
void attachWebContentsToIntent(Jack ThiesenThis is really creating a new window with a tab holding the specified web contents via a ReparentingTask, which is not clear from the current naming.
My thought is that something like `openWebContentsInNewWindow()` (open to better naming here) might be generally useful. It could look like this:
`openWebContentsInNewWindow(Consumer<Intent> intentDecorator, WebContents webContents, Profile profile, @NewWindowAppSource int source);` to be consistent with our other APIs.
Also on an aside, I'd really like to remove the #createNewWindowIntent() API TBH, since browser_window code is the only external caller currently that invokes this; MultiInstanceOrchestrator should ideally keep the new window intent creation package-private and expose APIs that can internally use this intent (with an optional `Consumer<Intent> intentDecorator` input that callers can use to set additional extras, flags etc.) to create new windows from tabs, links, web contents etc. - +@lin...@google.com for thoughts on this. I know this is a little tricky because of the way browser_window code is currently structured. Could we create a tracking bug to separately explore deprecating and eventually removing the #createNewWindowIntent() API in favor of completely delegating new window creation to the MIO view openNewWindow*() APIs?
Aishwarya RajeshWe agreed offline that the naming of this function is fine.
I've created this bug crbug.com/501427239 to track the second part of this comment and I also added a TODO in `ChromeAndroidTaskTrackerImpl`.
Thanks Jack!
I actually have a WIP CL for this cleanup here: crrev.com/c/7747418.
Sorry for not looking closely at this earlier; I feel like instead of simply doing #setupIntent() in the new MIO API method in this CL, we should invoke #moveTabsToNewWindow() from this API, that should ideally provide the ability to create a new window from a WebContents object end to end vs only setting up the intent - which is the primary problem we're trying to move away from even with #createNewWindowIntent().
I'd imagine this new API to be modeled around the #createNewWindow() API I'm adding in the above CL, but with an additional WebContents input. The MIO should be responsible for both creating the tab / intent as well as starting the activity.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Thiesen (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He added 4 comments![]( "Open in Gerrit")
"//chrome/android:chrome_java",`chrome/browser/ui/browser_window/internal:java` is a modularized target, so it shouldn't depend on the top-level `//chrome/android:chrome_java` target.
private static boolean createBrowserWindow(Add javadoc explaining the return value: what does `true` or `false` mean?
private static boolean createNormalBrowserWindow(ditto: javadoc for the return value.
private static boolean createPopupBrowserWindow(ditto: javadoc for the return value.
Related details
- Jack Thiesen
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 4 comments![]( "Open in Gerrit")
"//chrome/android:chrome_java",`chrome/browser/ui/browser_window/internal:java` is a modularized target, so it shouldn't depend on the top-level `//chrome/android:chrome_java` target.
Thanks for catching this. This was carryover from previous versions of this CL.
private static boolean createBrowserWindow(Add javadoc explaining the return value: what does `true` or `false` mean?
Done
private static boolean createNormalBrowserWindow(ditto: javadoc for the return value.
Done
private static boolean createPopupBrowserWindow(ditto: javadoc for the return value.
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He added 1 comment![]( "Open in Gerrit")
} catch (IllegalStateException e) {This looks like `MultiInstanceOrchestratorFactory.setInstanceForTesting()` isn't called early enough. Do we know why and can we call `setInstanceForTesting()` before the first `getInstance()`?
Related details
- Jack Thiesen
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
} catch (IllegalStateException e) {This looks like `MultiInstanceOrchestratorFactory.setInstanceForTesting()` isn't called early enough. Do we know why and can we call `setInstanceForTesting()` before the first `getInstance()`?
can we call setInstanceForTesting() before the first getInstance()
For tests, yes I think it is okay (and recommended) to set the mock orchestrator in test setup directly
Jack Thiesen (Gerrit)
Jack Thiesen added 1 comment![]( "Open in Gerrit")
Aishwarya RajeshThis looks like `MultiInstanceOrchestratorFactory.setInstanceForTesting()` isn't called early enough. Do we know why and can we call `setInstanceForTesting()` before the first `getInstance()`?
can we call setInstanceForTesting() before the first getInstance()
For tests, yes I think it is okay (and recommended) to set the mock orchestrator in test setup directly
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He voted Code-Review+1![]( "Open in Gerrit")
- Jack Thiesen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 6 comments![]( "Open in Gerrit")
Bug: 477944342is there a reason this bug needs to be restricted?
create_params.web_contents.release());I'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
auto* tab = tab_list_interface->GetActiveTab();let's also check that there's exactly one tab (also below)
std::unique_ptr<content::WebContents> web_contents;this is only supported on Android right now, right? Should we if-def this to only be on Android to make that clear and enforce it?
#include "content/public/browser/web_contents.h"nit: prefer a forward declaration instead of an include here
Related details
- Jack Thiesen
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 5 comments![]( "Open in Gerrit")
is there a reason this bug needs to be restricted?
Done
I'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Done
let's also check that there's exactly one tab (also below)
Done
this is only supported on Android right now, right? Should we if-def this to only be on Android to make that clear and enforce it?
Done
nit: prefer a forward declaration instead of an include here
Ack. This fails on compilation since we are using it in a `unique_ptr`.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
3 comments
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Done
This doesn't appear to have been done? (I don't see any difference between patch sets here)
#include "content/public/browser/web_contents.h"Jack Thiesennit: prefer a forward declaration instead of an include here
Ack. This fails on compilation since we are using it in a `unique_ptr`.
chatted offline; this should be doable -- we just need to include it in the proper other files
Related details
- Jack Thiesen
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 2 comments![]( "Open in Gerrit")
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
This doesn't appear to have been done? (I don't see any difference between patch sets here)
I put the requested comment in `create_browser_window.h` as a note.
#include "content/public/browser/web_contents.h"Jack Thiesennit: prefer a forward declaration instead of an include here
Devlin CroninAck. This fails on compilation since we are using it in a `unique_ptr`.
chatted offline; this should be doable -- we just need to include it in the proper other files
- Devlin Cronin
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
3 comments
// These must be last.I feel like there should be a different way to handle this, but I'll leave that up to android owners who probably know this better.
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
Jack ThiesenThis doesn't appear to have been done? (I don't see any difference between patch sets here)
I put the requested comment in `create_browser_window.h` as a note.
Please put one here, too. The `release()` here is alarming in C++-land : ) Also, the comment in create_browser_window.h just says "it's released to Java" -- but what is its lifetime? How do we ensure that it's properly destroyed before other things? (If e.g. we had a web contents that outlived its profile, that'd probably be bad)
Related details
- Jack Thiesen
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 2 comments![]( "Open in Gerrit")
// These must be last.I feel like there should be a different way to handle this, but I'll leave that up to android owners who probably know this better.
The reason why this is done is that these are declarations of files that will be automatically generated. They need the previous includes to have been declared before them or they won't know what to include themselves.
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
Jack ThiesenThis doesn't appear to have been done? (I don't see any difference between patch sets here)
Devlin CroninI put the requested comment in `create_browser_window.h` as a note.
Please put one here, too. The `release()` here is alarming in C++-land : ) Also, the comment in create_browser_window.h just says "it's released to Java" -- but what is its lifetime? How do we ensure that it's properly destroyed before other things? (If e.g. we had a web contents that outlived its profile, that'd probably be bad)
I don't agree that a comment belongs here.
This call is us creating a new `AndroidBrowserWindowCreateParams` Java object whose ctor requires that we pass it a raw pointer. The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object. That kind of contract is very common, specifically in JNI, but also in C++ land.
The only thing that we might be worried here is a C++ caller might not realize that the `WebContents` they passed in their `BrowserCreateParams` got invalidated and try to use it again later.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Thiesen (Gerrit)
Jack Thiesen added 1 comment![]( "Open in Gerrit")
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
Jack ThiesenThis doesn't appear to have been done? (I don't see any difference between patch sets here)
Devlin CroninI put the requested comment in `create_browser_window.h` as a note.
Jack ThiesenPlease put one here, too. The `release()` here is alarming in C++-land : ) Also, the comment in create_browser_window.h just says "it's released to Java" -- but what is its lifetime? How do we ensure that it's properly destroyed before other things? (If e.g. we had a web contents that outlived its profile, that'd probably be bad)
I don't agree that a comment belongs here.
This call is us creating a new `AndroidBrowserWindowCreateParams` Java object whose ctor requires that we pass it a raw pointer. The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object. That kind of contract is very common, specifically in JNI, but also in C++ land.
The only thing that we might be worried here is a C++ caller might not realize that the `WebContents` they passed in their `BrowserCreateParams` got invalidated and try to use it again later.
I added the comments and also patched what was a memory leak.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
Jack ThiesenThis doesn't appear to have been done? (I don't see any difference between patch sets here)
Devlin CroninI put the requested comment in `create_browser_window.h` as a note.
Jack ThiesenPlease put one here, too. The `release()` here is alarming in C++-land : ) Also, the comment in create_browser_window.h just says "it's released to Java" -- but what is its lifetime? How do we ensure that it's properly destroyed before other things? (If e.g. we had a web contents that outlived its profile, that'd probably be bad)
Jack ThiesenI don't agree that a comment belongs here.
This call is us creating a new `AndroidBrowserWindowCreateParams` Java object whose ctor requires that we pass it a raw pointer. The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object. That kind of contract is very common, specifically in JNI, but also in C++ land.
The only thing that we might be worried here is a C++ caller might not realize that the `WebContents` they passed in their `BrowserCreateParams` got invalidated and try to use it again later.
I added the comments and also patched what was a memory leak.
Thanks for adding these in and fixing the leak!
For future reference:
The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object
This is not at all common or guaranteed in chromium -- very, very many times, a raw pointer (either c-style or wrapped in raw_ptr) being passed into a class means that the class expects that item to _outlive_ the newly created object. As a concrete example, this is true for almost every instance of passing in a Profile* (or, on Desktop, a Browser*) to a ctor, which happens hundreds or even thousands of times in the codebase ([super naive search](https://source.chromium.org/search?q=%22raw_ptr%3CProfile%3E%22%20Profile%5C*%20f:.h$%20-f:test%20-f:out%20-f:%5Egen))
Related details
- Jack Thiesen
- Linyu He
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 1 comment![]( "Open in Gerrit")
create_params.web_contents.release());Jack ThiesenI'm guessing we can't pass this as a unique_ptr with std::move()?
If not, please add a comment about what manages the lifetime of the WebContents; as it stands, this looks like a recipe for a leak.
Devlin CroninDone
Jack ThiesenThis doesn't appear to have been done? (I don't see any difference between patch sets here)
Devlin CroninI put the requested comment in `create_browser_window.h` as a note.
Jack ThiesenPlease put one here, too. The `release()` here is alarming in C++-land : ) Also, the comment in create_browser_window.h just says "it's released to Java" -- but what is its lifetime? How do we ensure that it's properly destroyed before other things? (If e.g. we had a web contents that outlived its profile, that'd probably be bad)
Jack ThiesenI don't agree that a comment belongs here.
This call is us creating a new `AndroidBrowserWindowCreateParams` Java object whose ctor requires that we pass it a raw pointer. The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object. That kind of contract is very common, specifically in JNI, but also in C++ land.
The only thing that we might be worried here is a C++ caller might not realize that the `WebContents` they passed in their `BrowserCreateParams` got invalidated and try to use it again later.
Devlin CroninI added the comments and also patched what was a memory leak.
Thanks for adding these in and fixing the leak!
For future reference:
The new object, by accepting a raw pointer, also accepts responsibility for the lifetime of this object
This is not at all common or guaranteed in chromium -- very, very many times, a raw pointer (either c-style or wrapped in raw_ptr) being passed into a class means that the class expects that item to _outlive_ the newly created object. As a concrete example, this is true for almost every instance of passing in a Profile* (or, on Desktop, a Browser*) to a ctor, which happens hundreds or even thousands of times in the codebase ([super naive search](https://source.chromium.org/search?q=%22raw_ptr%3CProfile%3E%22%20Profile%5C*%20f:.h$%20-f:test%20-f:out%20-f:%5Egen))
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He added 1 comment![]( "Open in Gerrit")
int64_t window_ptr =
Java_BrowserWindowCreatorBridge_createBrowserWindow(env, j_create_params);
if (window_ptr != 0) {
// The Java Tab has taken ownership.
create_params.web_contents.release();
}Strictly speaking, this isn't correct. For the sync `CreateBrowserWindow()`, the returned `window_ptr` is like an empty shell, i.e., there is no tab or feature inside the object.
Do we have to support the sync `CreateBrowserWindow()` for `create_params.web_contents` right now? If not, I would suggest immediately return `nullptr` if `create_params.web_contents` is non-null.
Related details
- Jack Thiesen
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. |
Linyu He (Gerrit)
Linyu He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Linyu He
| Code-Review | +1 |
1 comment
int64_t window_ptr =
Java_BrowserWindowCreatorBridge_createBrowserWindow(env, j_create_params);
if (window_ptr != 0) {
// The Java Tab has taken ownership.
create_params.web_contents.release();
}Strictly speaking, this isn't correct. For the sync `CreateBrowserWindow()`, the returned `window_ptr` is like an empty shell, i.e., there is no tab or feature inside the object.
Do we have to support the sync `CreateBrowserWindow()` for `create_params.web_contents` right now? If not, I would suggest immediately return `nullptr` if `create_params.web_contents` is non-null.
Chatted offline; so there is something in Java that holds `create_params.web_contents`, but it's not a tab as described at Line 40.
We should clarify the comment.
Calder Kitagawa (Gerrit)
Calder Kitagawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
2 comments
private final @Nullable WebContents mWebContents;This java WebContent's will own the pointer to the C++ WebContents.
We do need to make sure that this `AndroidBrowserWindowCreateParamsImpl` always is consumed or if not it needs to destroy the `mWebContents` as it is the explicit owner of that object now.
In response to https://chromium-review.git.corp.google.com/c/chromium/src/+/7744757/comment/010eccdd_7b4a3367/
.createNewWindowFromWebContents(We should probably metion on the Javadoc for this method and createNewPopupFromWebContents that these take ownership of the WebContents.
Related details
- Jack Thiesen
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. |
Jack Thiesen (Gerrit)
Jack Thiesen added 3 comments![]( "Open in Gerrit")
int64_t window_ptr =
Java_BrowserWindowCreatorBridge_createBrowserWindow(env, j_create_params);
if (window_ptr != 0) {
// The Java Tab has taken ownership.
create_params.web_contents.release();
}Linyu HeStrictly speaking, this isn't correct. For the sync `CreateBrowserWindow()`, the returned `window_ptr` is like an empty shell, i.e., there is no tab or feature inside the object.
Do we have to support the sync `CreateBrowserWindow()` for `create_params.web_contents` right now? If not, I would suggest immediately return `nullptr` if `create_params.web_contents` is non-null.
Chatted offline; so there is something in Java that holds `create_params.web_contents`, but it's not a tab as described at Line 40.
We should clarify the comment.
Done
This java WebContent's will own the pointer to the C++ WebContents.
We do need to make sure that this `AndroidBrowserWindowCreateParamsImpl` always is consumed or if not it needs to destroy the `mWebContents` as it is the explicit owner of that object now.
In response to https://chromium-review.git.corp.google.com/c/chromium/src/+/7744757/comment/010eccdd_7b4a3367/
Marked as resolved.
We should probably metion on the Javadoc for this method and createNewPopupFromWebContents that these take ownership of the WebContents.
I will add this in a followup.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Thiesen (Gerrit)
Jack Thiesen 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
22 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/browser_window/internal/android/create_browser_window_android.cc
Insertions: 8, Deletions: 6.
@@ -23,8 +23,8 @@
JNIEnv* env = base::android::AttachCurrentThread();
const gfx::Rect& bounds = create_params.initial_bounds;
- // Ownership of the WebContents is retained in C++ until the window creation
- // is successful.
+ // This code is still responsible for the WebContents until it receives a
+ // signal that the window creation is possible.
base::android::ScopedJavaLocalRef<jobject> j_create_params =
Java_AndroidBrowserWindowCreateParamsImpl_create(
env, static_cast<int>(create_params.type),
@@ -37,7 +37,8 @@
Java_BrowserWindowCreatorBridge_createBrowserWindow(env, j_create_params);
if (window_ptr != 0) {
- // The Java Tab has taken ownership.
+ // Java has created a detached Tab which has assumed ownership of this
+ // WebContents (and is being reparented asynchronously).
create_params.web_contents.release();
}
@@ -50,8 +51,8 @@
JNIEnv* env = base::android::AttachCurrentThread();
const gfx::Rect& bounds = create_params.initial_bounds;
- // Ownership of the WebContents is retained in C++ until the window creation
- // is successful.
+ // This code is still responsible for the WebContents until it receives a
+ // signal that the window creation is possible.
base::android::ScopedJavaLocalRef<jobject> j_create_params =
Java_AndroidBrowserWindowCreateParamsImpl_create(
env, static_cast<int>(create_params.type),
@@ -66,7 +67,8 @@
[](base::OnceCallback<void(BrowserWindowInterface*)> cb,
std::unique_ptr<content::WebContents> web_contents, int64_t ptr) {
if (ptr != 0) {
- // Java will assume ownership of this WebContents.
+ // Java has created a detached Tab which has assumed ownership of this
+ // WebContents (and is being reparented asynchronously).
web_contents.release();
}
std::move(cb).Run(reinterpret_cast<BrowserWindowInterface*>(ptr));
```
Change information
CreateBrowserWindow() Accepts a WebContents On Android
Sometimes a new browser window should be created using an existing
WebContents as its first tab. This change enables Android to handle such
a call from the C++. This is accomplished by:
1. Updating the BrowserWindowCreateParams to hold the WebContents
2. Calling the MultiInstanceOrchestrator or PopupCreator to handle
creating a new window/popup using the provided WebContents.
This change also patches a silent failure inside of
ChromeAndroidTaskTrackerImpl#createPendingTask() where the callback
might never receive a signal. Browser testing is included.
- M chrome/browser/ui/browser_window/internal/BUILD.gn
- M chrome/browser/ui/browser_window/internal/android/create_browser_window_android.cc
- M chrome/browser/ui/browser_window/internal/android/create_browser_window_android_browsertest.cc
- M chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/AndroidBrowserWindowCreateParamsImpl.java
- M chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/AndroidBrowserWindowCreateParamsImplUnitTest.java
- M chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/ChromeAndroidTaskIntegrationTest.java
- M chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/ChromeAndroidTaskTrackerImpl.java
- M chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/ChromeAndroidTaskTrackerImplUnitTest.java
- M chrome/browser/ui/browser_window/internal/create_browser_window.cc
- M chrome/browser/ui/browser_window/public/android/BUILD.gn
- M chrome/browser/ui/browser_window/public/android/java/src/org/chromium/chrome/browser/ui/browser_window/AndroidBrowserWindowCreateParams.java
- M chrome/browser/ui/browser_window/public/create_browser_window.h
- M chrome/browser/ui/browser_window/test/BUILD.gn
- M chrome/browser/ui/browser_window/test/android/java/src/org/chromium/chrome/browser/ui/browser_window/ChromeAndroidTaskUnitTestSupport.java
Code-Review: +1 by Calder Kitagawa, +1 by Linyu He, +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |