[PWA/Nav Capturing] Add middle click support [chromium/src : main]
2 views
Skip to first unread message
Dibyajyoti Pal (Gerrit)
Aug 6, 2024, 4:38:45 PM8/6/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Daniel Murphy
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Dibyajyoti Pal . unresolved
PTAL dmurph@ at the overall implementation (mek@ added as cc for eyes).
There might be some test failures for the redirection cases, but `kScopeA2A` and `kScopeA2B` behavior should be working fine.
Related details
Attention is currently required from:
- Daniel Murphy
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 6, 2024, 5:14:48 PM8/6/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Daniel Murphy and Dibyajyoti Pal
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Dibyajyoti Pal . unresolved
PTAL dmurph@ at the overall implementation (mek@ added as cc for eyes).
There might be some test failures for the redirection cases, but `kScopeA2A` and `kScopeA2B` behavior should be working fine.
Dibyajyoti Pal
PTAL, handling the build failure locally (should be a simple missing include in the test) and will be uploading everything soon.
Related details
Attention is currently required from:
- Daniel Murphy
- Dibyajyoti Pal
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 7, 2024, 12:47:21 PM8/7/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Daniel Murphy
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Dibyajyoti PalPTAL dmurph@ at the overall implementation (mek@ added as cc for eyes).
There might be some test failures for the redirection cases, but `kScopeA2A` and `kScopeA2B` behavior should be working fine.
PTAL, handling the build failure locally (should be a simple missing include in the test) and will be uploading everything soon.
Dibyajyoti Pal
Turns out the failing tests are for redirects, and all other middle clicks seem to work fine. I will disable the tests so that those can be handled while redirects are being worked on.
Related details
Attention is currently required from:
- Daniel Murphy
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Aug 7, 2024, 1:55:54 PM8/7/24
to Dibyajyoti Pal, Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal
Daniel Murphy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
3 comments
File chrome/browser/web_applications/web_app_link_capturing_parameterized_browsertest.cc
Line 753, Patchset 10:
// Even though usage of `chrome::FindBrowserWithTab()` is technically not
// allowed anymore, this is needed because sometimes clicks can launch a new
// tab inside an existing window.Daniel Murphy . unresolved
The comment seems to be mostly talking about production use-cases, not in tests. So you can probably remove this comment.
Line 798, Patchset 10:
// Even though usage of `chrome::FindBrowserWithTab()` is technically not
// allowed anymore, this is needed because sometimes clicks can launch a new
// tab inside an existing window.Daniel Murphy . unresolved
same, can remove
File chrome/test/data/web_apps/link_capture_test_input.json
Line 1553, Patchset 11 (Latest):
"target": "FRAME"Daniel Murphy . resolved
I see the 'link' type is correct here - so perhaps the window.open API doesn't get events like the anchor links do / doesn't get mouse stuff / keyboard stuff?
That's probably OK... but we should maybe investigate what 'aux' style things can actually be sent / read from window.open.
Resolving - it looks like btn window.open links should never hit the aux-click stuff.
Related details
Attention is currently required from:
- Dibyajyoti Pal
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 7, 2024, 2:07:00 PM8/7/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
File chrome/browser/web_applications/web_app_link_capturing_parameterized_browsertest.cc
Line 753, Patchset 10:
// Even though usage of `chrome::FindBrowserWithTab()` is technically not
// allowed anymore, this is needed because sometimes clicks can launch a new
// tab inside an existing window.Daniel Murphy . resolved
The comment seems to be mostly talking about production use-cases, not in tests. So you can probably remove this comment.
Dibyajyoti Pal
Done
Line 798, Patchset 10:
// Even though usage of `chrome::FindBrowserWithTab()` is technically not
// allowed anymore, this is needed because sometimes clicks can launch a new
// tab inside an existing window.Daniel Murphy . resolved
Dibyajyoti Palsame, can remove
Done
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 7, 2024, 2:07:00 PM8/7/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 7, 2024, 5:32:23 PM8/7/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal (Gerrit)
Aug 8, 2024, 11:35:30 AM8/8/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal (Gerrit)
Aug 8, 2024, 12:01:37 PM8/8/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 8, 2024, 1:05:20 PM8/8/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 8, 2024, 1:05:52 PM8/8/24
to Daniel Murphy, Marijn Kruisselbrink, Chromium LUCI CQ, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Dibyajyoti Pal . resolved
Dibyajyoti PalPTAL dmurph@ at the overall implementation (mek@ added as cc for eyes).
There might be some test failures for the redirection cases, but `kScopeA2A` and `kScopeA2B` behavior should be working fine.
Dibyajyoti PalPTAL, handling the build failure locally (should be a simple missing include in the test) and will be uploading everything soon.
Turns out the failing tests are for redirects, and all other middle clicks seem to work fine. I will disable the tests so that those can be handled while redirects are being worked on.
Dibyajyoti Pal
Flakiness seems to be an unrelated infra failure on CrOS, failed crbug.com/358296948
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Aug 8, 2024, 2:31:20 PM8/8/24
to Dibyajyoti Pal, Daniel Murphy, Marijn Kruisselbrink, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/web_applications/web_app_link_capturing_parameterized_browsertest.cc
Insertions: 0, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/test/data/banners/link_capturing/scope_a/start.html
Insertions: 9, Deletions: 12.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
[PWA/Nav Capturing] Add middle click support
This CL adds support for middle clicks to be captured for navigation
capturing by PWAs, as per the behavior outlined in
https://docs.google.com/document/d/176tREY7_qmVsRt15z4q6BRhQyfJAUvy__DmEMDbQ8LI/edit?pli=1#heading=h.3r1ull9cvqat.
Middle clicks simulated and triggered from JS inside a test is
different from "real" user initiated middle clicks based on how
the blink layers parses it to output a NavigationPolicy. To keep
the behaviors consistent, this CL also modifies the existing link
capturing browsertests to use a link click behavior that is
closer to production as much as possible.
WebAppLinkCapturingParameterizedBrowsertest.
Bug: 356234778, 351775835
Change-Id: I45f1ebfba4dee55d08c721760294d217b57c3820
Include-Ci-Only-Tests: true
Low-Coverage-Reason: launch utitlities added verified via
Validate-Test-Flakiness: skip
Reviewed-by: Daniel Murphy <dmu...@chromium.org>
Commit-Queue: Dibyajyoti Pal <diby...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1339198}
Files:
- M chrome/browser/ui/web_applications/web_app_launch_utils.cc
- M chrome/browser/web_applications/web_app_link_capturing_parameterized_browsertest.cc
- M chrome/test/data/banners/link_capturing/scope_a/start.html
- M chrome/test/data/web_apps/link_capture_test_input.json
Change size: M
Delta: 4 files changed, 123 insertions(+), 101 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Daniel Murphy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marijn Kruisselbrink (Gerrit)
Aug 13, 2024, 5:49:55 PM8/13/24
to Chromium LUCI CQ, Dibyajyoti Pal, Daniel Murphy, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Attention needed from Dibyajyoti Pal
Marijn Kruisselbrink added 1 comment![]( "Open in Gerrit")
File chrome/browser/ui/web_applications/web_app_launch_utils.cc
Line 932, Patchset 16 (Latest):
if (!params.browser->app_controller()->ShouldHideNewTabButton()) {Marijn Kruisselbrink . unresolved
post-land-nit: ShouldHideNewTabButton is not the right way to verify if an app is in tabbed mode or not, as it is possible to have a tabbed PWA without a new-tab button. You probably meant app_controller()->has_tab_strip()?
Related details
Attention is currently required from:
- Dibyajyoti Pal
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Aug 13, 2024, 5:59:12 PM8/13/24
to Chromium LUCI CQ, Daniel Murphy, Marijn Kruisselbrink, chromium...@chromium.org, alancutter...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dominickn+watch-...@chromium.org, ericwillige...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mgiuca...@chromium.org, philli...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
File chrome/browser/ui/web_applications/web_app_launch_utils.cc
Line 932, Patchset 16 (Latest):
if (!params.browser->app_controller()->ShouldHideNewTabButton()) {Marijn Kruisselbrink . resolved
post-land-nit: ShouldHideNewTabButton is not the right way to verify if an app is in tabbed mode or not, as it is possible to have a tabbed PWA without a new-tab button. You probably meant app_controller()->has_tab_strip()?
Dibyajyoti Pal
Ack, will fix this in a follow-up CL.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages