Add Ctrl+Alt+Click to open links in split view [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hi, @foo...@chromium.org - not really sure how to send a CL that is a feature request, could you please guide me in the correct direction? is there some design-doc, i2s or other protocol - like just assigning reviewers feels wrong?
Related details
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
hi, @foo...@chromium.org - not really sure how to send a CL that is a feature request, could you please guide me in the correct direction? is there some design-doc, i2s or other protocol - like just assigning reviewers feels wrong?
Hey Helmut, I looked through recent Chromium commits to find someone who has worked on split view, and sent an email to ask about how to proceed.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gary Klassen (Gerrit)
Gary Klassen added 1 comment![]( "Open in Gerrit")
Love this. I was just thinking I also really wanted this (so I can open a link and see the source and target pages at the same time). I had landed on the same implementation and started to make the change when a colleague showed me your change which is already much more complete than mine. My suggestion is to flag guard it and make it available in chrome://flags and then post for review. This can help to build understanding and confidence with landing the change. I am happy to help along with foolip to support, but of course the decision is with reviewers. Also, it is also much easier to land smaller changes with fewer reviewers and so I'd recommend splitting into smaller changes with a bug with a quick design to show how the changes make sense together.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hello alex...@google.com this is basically feature complete, please let me know if you want me to address anything.
Related details
- Alex Tseng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
Adding agale@, who is an owner of this feature (OOO until Tuesday for the holiday).
Related details
- Alex Tseng
- Alison Gale
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
Alison Gale added 1 comment![]( "Open in Gerrit")
Sorry for the delay on the review. As of now, the feedback from our product team is that we only want to introduce shortcuts like this when we have a good story of how we will educate users about their existence. This will take some time (hoping in the next few months but can't guarantee anything). But it is definitely something we want to do.
Related details
- Alex Tseng
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From analysis/uma/chrome-metrics.gwsq:
Histograms should by default be reviewed by the owners of the subdirectories. The chromium-met...@google.com gwsq should be used when there are no individual owners, or for escalation to the Metrics team.
If you are interested in becoming a metrics reviewer, please review the instructions at https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Becoming-a-Metrics-Reviewer
Reviewer source(s):
lucn...@google.com is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Alex Tseng
- Dana Fried
- Daniel Cheng
- Elly FJ
- Kent Tamura
- Luc Nguyen
- Yanzhu Du
- Zijie He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
Alison Gale voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alison Gale
| Code-Review | -1 |
1 comment
As I mentioned in my comment, the PM for the feature would like to wait on this until we have a comprehensive plan for how to educate users about this feature. Removing all of the reviews to avoid wasting their time.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
As I mentioned in my comment, the PM for the feature would like to wait on this until we have a comprehensive plan for how to educate users about this feature. Removing all of the reviews to avoid wasting their time.
ok sorry from the last reply of @at...@google.com ok the bug i thought its good to proceed behind flag. whatever - as before ill be ready and will keep cl mergeable
Related details
- Alison Gale
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
Alison Gale voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alison Gale
| Code-Review | +0 |
1 comment
Helmut JanuschkaAs I mentioned in my comment, the PM for the feature would like to wait on this until we have a comprehensive plan for how to educate users about this feature. Removing all of the reviews to avoid wasting their time.
ok sorry from the last reply of @at...@google.com ok the bug i thought its good to proceed behind flag. whatever - as before ill be ready and will keep cl mergeable
Sorry for the miscommunication. The eng team wasn't notified of the approval but I found the bug and see the conversation there so I guess this is okay to proceed. But this is a lot of reviewers to add at once. I'd start with maybe 3 owners needed for blink, content renderer, and mojom changes. And then add the other owners once you get alignment. This will help reduce the review load if any changes are needed.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Gale (Gerrit)
Alison Gale added 1 comment![]( "Open in Gerrit")
I'm not familiar with any of the code outside of chrome/browser but have fixed a handful of security bugs in split view. Can you confirm that the bug described in crbug.com/426480606 isn't an issue here? That will ensure the correct referrer is being set.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
I'm not familiar with any of the code outside of chrome/browser but have fixed a handful of security bugs in split view. Can you confirm that the bug described in crbug.com/426480606 isn't an issue here? That will ensure the correct referrer is being set.
I took a look and I'm confident crbug.com/426480606 is not an issue here.
That bug was specific to the context menu "Open in Split View" code path in `RenderViewContextMenu::OpenLinkInSplitView()`, which was manually constructing `LoadURLParams` without calling `CreateReferrer()`.
this CL adds Ctrl+Alt+Click which goes through an entirely different path, Blink's standard link-click navigation pipeline. In this flow, the referrer is automatically populated by Blink from the `WebURLRequest` in `RenderFrameImpl::OpenURL()`, just like it is for Ctrl+Click or Shift+Click. browser-side code in `Browser::OpenURLFromTab()` properly forwards it:
```cpp
load_params.referrer = content::Referrer(params.referrer.url, params.referrer.policy);
```
So the referrer is correctly set for both the "already split" path and the "new split" path
but looping someone from security in, before shipping would be a good thing i guess (just in case)
@dlj...@chromium.org and @rob...@chromium.org, as per comment on the issue, thank you in advance for your time to review this CL, please let me know if you want me to address anything.
Related details
- Alison Gale
- Darryl James
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
Hi! Give me another day to get to this review. If this needs to expedited / prioritized higher please let me know!
Thank you!
Related details
- Alison Gale
- Helmut Januschka
- Robert Liao
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. |
Alison Gale (Gerrit)
Alison Gale added 5 comments![]( "Open in Gerrit")
"owners": [ "hel...@januschka.com" ], "Enable split view link opening";nit: I would call this "Enable split view link opening shortcut" For variable names and the strings. You can already open links in split view, this just adds a handy shortcut
(!source_tab || !source_tab->IsSplit())) {What would happen if source_tab is undefined because that tab got closed and the tab at new_tab_index happened to be the new active tab?
<int value="11" label="Link click (Ctrl+Alt+Click)"/>nit: probably would just do "Link click shortcut" rather than including the keys which are different per OS"
case WindowOpenDisposition::SWITCH_TO_TAB:Are you making changes to the SWITCH_TO_TAB functionality?
Related details
- Helmut Januschka
- Robert Liao
Robert Liao (Gerrit)
Robert Liao added 1 comment![]( "Open in Gerrit")
if (new_tab_modifier && alt_key && !shift_key &&new_tab_modifier includes the middle button, which means you can open a split view without holding Ctrl/Meta. Intended?
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 7 comments![]( "Open in Gerrit")
Hi! Give me another day to get to this review. If this needs to expedited / prioritized higher please let me know!
Thank you!
Done
Can you add top-chrome...@google.com as a secondary owner?
Done
nit: I would call this "Enable split view link opening shortcut" For variable names and the strings. You can already open links in split view, this just adds a handy shortcut
Done
What would happen if source_tab is undefined because that tab got closed and the tab at new_tab_index happened to be the new active tab?
In that case `AddToNewSplit()` would hit `CHECK(active_index() != indices[0])`, because it cannot split a tab with itself. Normally `NEW_SPLIT_VIEW` clears `ADD_ACTIVE`, so the new tab should not become active.
nit: probably would just do "Link click shortcut" rather than including the keys which are different per OS"
Done
Are you making changes to the SWITCH_TO_TAB functionality?
reverted. now only adds NEW_SPLIT_VIEW, sorry
new_tab_modifier includes the middle button, which means you can open a split view without holding Ctrl/Meta. Intended?
good call, i initially thought its good to align it with the new tab behaviour,but undone it, guess its easier if we do not support it for now
Related details
- Alison Gale
- Darryl James
- Robert Liao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 3 comments![]( "Open in Gerrit")
tab->GetContents()->GetController().LoadURLWithParams(load_params);Random question: Let's say I have a gerrit bug half way filled out, and the split tab would navigate it, does the unload handler still trigger?
An unload handler is the thing that basically says "woah wait a minute you have unsaved work! Are you sure you want to exit this page?!?!"
return source;I don't know if this is exactly a bug, but I would expect this function to return the `WebContents` that was navigated. In this case, `tab->GetContents()`.
Otherwise, `OpenURL` will return the untouched tab even though it did navigate. This might cause confusion among existing callers trying to use the returned value.
What kinds of problems was `DidOpenRequestedURL` causing when this block is triggered?
return kNavigationPolicyCurrentTab;@ag...@chromium.org - What are your thoughts here?
Essentially, if a website overrides the policy (ex: target="_blank"), then this will treat the split view command as a current tab navigation. Is that okay? Or should it open a new tab (and immediately make it the active tab, or open it in the background)?
Related details
- Alison Gale
- Helmut Januschka
- Robert Liao
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. |
Alison Gale (Gerrit)
Alison Gale added 1 comment![]( "Open in Gerrit")
return kNavigationPolicyCurrentTab;@ag...@chromium.org - What are your thoughts here?
Essentially, if a website overrides the policy (ex: target="_blank"), then this will treat the split view command as a current tab navigation. Is that okay? Or should it open a new tab (and immediately make it the active tab, or open it in the background)?
For open link in split view, if the active tab isn't in a split then it will always open the link in a new tab (regardless of the target attr) and then create a split. If the active tab is in a split then it will always navigate the other tab to that link, even if the target indicates it should open in a new tab. That seems to be consistent with the other "open link..." options.
Related details
- Helmut Januschka
- Robert Liao
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
tab->GetContents()->GetController().LoadURLWithParams(load_params);Random question: Let's say I have a gerrit bug half way filled out, and the split tab would navigate it, does the unload handler still trigger?
An unload handler is the thing that basically says "woah wait a minute you have unsaved work! Are you sure you want to exit this page?!?!"
good call, dont think this is an issue, still goes through normal beforeunload handling for the pane being navigated.
I don't know if this is exactly a bug, but I would expect this function to return the `WebContents` that was navigated. In this case, `tab->GetContents()`.
Otherwise, `OpenURL` will return the untouched tab even though it did navigate. This might cause confusion among existing callers trying to use the returned value.
What kinds of problems was `DidOpenRequestedURL` causing when this block is triggered?
Good point. I tried returning `tab->GetContents()`, but that causes `WebContentsImpl::OpenURL()` to fire `DidOpenRequestedURL` for an already-existing tab. On repeated split opens this hits a DCHECK in `HistoryTabHelper`
So in the already-split path its intentionally returning `source` while navigating the other pane directly. This keeps the behavior aligned with the context-menu "Open link in split view" path, which also reuses and navigates the existing other pane rather than treating it as a newly opened tab.
Alison Gale@ag...@chromium.org - What are your thoughts here?
Essentially, if a website overrides the policy (ex: target="_blank"), then this will treat the split view command as a current tab navigation. Is that okay? Or should it open a new tab (and immediately make it the active tab, or open it in the background)?
For open link in split view, if the active tab isn't in a split then it will always open the link in a new tab (regardless of the target attr) and then create a split. If the active tab is in a split then it will always navigate the other tab to that link, even if the target indicates it should open in a new tab. That seems to be consistent with the other "open link..." options.
it is consistent with existing behaviour.
case ui::mojom::WindowOpenDisposition::SWITCH_TO_TAB:bots moaned about missing enum
return ui::mojom::WindowOpenDisposition::SWITCH_TO_TAB;bots moaned about missing enum
Related details
- Alison Gale
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
Related details
- Alison Gale
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@ag...@chromium.org to not flood reviewers, do you have someone in mind for the remaining files? (using suggest reviewers, feels like ending up in way too many) appreciate any guidance on whom to pick. thanks!
Related details
- Alison Gale
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
return kNavigationPolicyCurrentTab;Though it's kind of repetitive, probably worth stating here that we don't want to allow synthesized events to trigger split view.
(Do we have a test for any of this)
return ui::mojom::WindowOpenDisposition::SWITCH_TO_TAB;bots moaned about missing enum
It's unclear if this was ever meant to be included. Do you know what the history of this is, and why this plumbing wasn't originally added? It'd be good to confirm that this was a mistake and note intentionally left out.
Related details
- Alison Gale
- Helmut Januschka
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. |
Alison Gale (Gerrit)
Alison Gale added 1 comment![]( "Open in Gerrit")
@ag...@chromium.org to not flood reviewers, do you have someone in mind for the remaining files? (using suggest reviewers, feels like ending up in way too many) appreciate any guidance on whom to pick. thanks!
I would add a specific OWNER for third_party/blink/renderer/core/loader/OWNERS and add chrome-ip...@google.com for mojom. After that, the changes are very boilerplate but the files are very distinct so you likely will need a lot of OWNERS. It's just easier to do them a few at a time for the more complex files first.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@jap...@chromium.org - thanks in advance, adding you for `third_party/blink/renderer/core/loader` let me know if you want me to address anything.
Related details
- Chromium IPC Reviews
- Nate Chapin
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mk...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mk...@chromium.org
Reviewer source(s):
mk...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Mike West
- Nate Chapin
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. |