[PWA][3/N] Extensions commit launch params post navigation commit [chromium/src : main]
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
PTAL, thanks!
+finnur@ for `c/b/extensions`
+dmurph@ for the rest of the code.
Related details
- Chromium Web Applications Reviews
- Daniel Murphy
- Finnur Thorarinsson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Finnur Thorarinsson
| Code-Review | +1 |
1 comment
- Daniel Murphy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
const std::optional<webapps::AppId>& pending_launch_app_id() const {Suggested Architecture Improvement:
Instead of storing a single `pending_launch_app_id_` on `WebAppTabHelper` (which is prone to race conditions if launches overlap), we should track the active launch navigation using a `base::WeakPtr` to `WebAppLaunchNavigationHandleUserData`.
**Proof of Single Navigation Handle**:
As confirmed in `content/browser/renderer_host/frame_tree_node.h`, a `FrameTreeNode` owns at most one active `NavigationRequest` (which implements `NavigationHandle`) for the frame:
`std::unique_ptr<NavigationRequest> navigation_request_;`
Starting a new navigation cancels the previous one (see `FrameTreeNode::ResetNavigationRequest`). Thus, for the top-level frame (which app launches target), there is at most one active navigation handle at a time.
**Proposed Design**:
1. `WebAppLaunchNavigationHandleUserData` implements a `WeakPtrFactory`.
2. On `SetLaunchParams`, it registers its `WeakPtr` with `WebAppTabHelper`: `tab_helper->SetPendingLaunch(GetWeakPtr())`.
3. `WebAppTabHelper` stores: `base::WeakPtr<WebAppLaunchNavigationHandleUserData> pending_launch_;`.
4. `WebAppLaunchProcess::MaybeNavigateBrowser` checks `tab_helper->GetPendingLaunchForApp(app_id)`.
5. If found, it replaces the old params with the new one (that's the design we want, right?)
6. On commit, the user data dispatches the params to the simplified `LaunchQueue`.
7. On destruction of the navigation handle, `pending_launch_` automatically becomes null.
Related details
- Dibyajyoti Pal
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
const std::optional<webapps::AppId>& pending_launch_app_id() const {Suggested Architecture Improvement:
Instead of storing a single `pending_launch_app_id_` on `WebAppTabHelper` (which is prone to race conditions if launches overlap), we should track the active launch navigation using a `base::WeakPtr` to `WebAppLaunchNavigationHandleUserData`.
**Proof of Single Navigation Handle**:
As confirmed in `content/browser/renderer_host/frame_tree_node.h`, a `FrameTreeNode` owns at most one active `NavigationRequest` (which implements `NavigationHandle`) for the frame:
`std::unique_ptr<NavigationRequest> navigation_request_;`
Starting a new navigation cancels the previous one (see `FrameTreeNode::ResetNavigationRequest`). Thus, for the top-level frame (which app launches target), there is at most one active navigation handle at a time.**Proposed Design**:
1. `WebAppLaunchNavigationHandleUserData` implements a `WeakPtrFactory`.
2. On `SetLaunchParams`, it registers its `WeakPtr` with `WebAppTabHelper`: `tab_helper->SetPendingLaunch(GetWeakPtr())`.
3. `WebAppTabHelper` stores: `base::WeakPtr<WebAppLaunchNavigationHandleUserData> pending_launch_;`.
4. `WebAppLaunchProcess::MaybeNavigateBrowser` checks `tab_helper->GetPendingLaunchForApp(app_id)`.
5. If found, it replaces the old params with the new one (that's the design we want, right?)
6. On commit, the user data dispatches the params to the simplified `LaunchQueue`.
7. On destruction of the navigation handle, `pending_launch_` automatically becomes null.
Oooo using the navigation handle to track the pending app_id for navigation, love that idea! Implemented.
The only issue is that the `WebAppLaunchNavigationHandleUserData` lies in `c/b/ui/`, so we can't include it directly inside `c/b/web_applications/`, so I worked around it by creating a pure virtual `WebAppLaunchParamsHolder` class inside `c/b/web_applications/` that `WebAppLaunchNavigationHandleUserData` implements. This seems to work, PTAL and let me know if there is perhaps a simpler way to do it.
Related details
- Daniel Murphy
- Finnur Thorarinsson
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
*/extensions/* LGTM
PTAL, I made an architectural change that uploaded new files from before, and wiped out the +1s. The extensions changes are still the same, can you PTAL again? Thanks!