[inline] Make site settings permanently enabled and default CTA to explain [chromium/src : main]
0 views
Skip to first unread message
Dave Tapuska (Gerrit)
Jul 9, 2026, 10:25:54 AM (2 days ago) Jul 9
to Gary Klassen, Sirui Wu, Chromium Metrics Reviews, chromium...@chromium.org, wuis...@google.com, wus...@chromium.org, asvitki...@chromium.org, asvitkine...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, jmedle...@chromium.org
Attention needed from Gary Klassen and Sirui Wu
Dave Tapuska added 2 comments![]( "Open in Gerrit")
File chrome/browser/glic/glic_selection_observer.cc
Line 671, Patchset 16 (Latest):
LOG(ERROR) << "GlicSelectionObserver: closing widget in UpdateSelectionState (panel_showing)";Dave Tapuska . unresolved
Remove
Line 697, Patchset 16 (Latest):
if (!ShouldShowSelectionWidget()) {Dave Tapuska . unresolved
This all looks like it does un-necessary code when we are in the updates only mode... We now request teh text bounds, create the selection widget, etc. Did you test the updates only mode?
Related details
Attention is currently required from:
- Gary Klassen
- Sirui Wu
Submit Requirements:
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. |
Gary Klassen (Gerrit)
Jul 9, 2026, 2:29:28 PM (2 days ago) Jul 9
to Sirui Wu, Dave Tapuska, Chromium Metrics Reviews, chromium...@chromium.org, wuis...@google.com, wus...@chromium.org, asvitki...@chromium.org, asvitkine...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, jmedle...@chromium.org
Attention needed from Dave Tapuska and Sirui Wu
Gary Klassen added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 17 (Latest):
Gary Klassen . resolved
Thanks for the review! Comments addressed. Look ok?
File chrome/browser/glic/glic_selection_observer.cc
Line 671, Patchset 16:
LOG(ERROR) << "GlicSelectionObserver: closing widget in UpdateSelectionState (panel_showing)";Dave Tapuska . resolved
Gary KlassenRemove
Done
This all looks like it does un-necessary code when we are in the updates only mode... We now request teh text bounds, create the selection widget, etc. Did you test the updates only mode?
Gary Klassen
Thanks! UpdateSelectionState() was guarding calls to ShowSelectionAffordance() with !updates_only, and we have unit tests covering updates_only mode (UpdateSelectionStatePanelShowingNoWidget / UpdateSelectionStatePanelShowingUpdatesOnly). To make the contract completely bulletproof and self-documenting, early returns have been added at the top of ShowSelectionAffordance() and ShouldShowSelectionWidget() so updates_only mode is guaranteed to never compute text bounds or create widget UI.
Related details
Attention is currently required from:
- Dave Tapuska
- Sirui Wu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Jul 10, 2026, 10:10:02 AM (yesterday) Jul 10
to Gary Klassen, android-bu...@system.gserviceaccount.com, Ian Vollick, Sirui Wu, Chromium Metrics Reviews, chromium...@chromium.org, devtools...@chromium.org, net-r...@chromium.org, cblume...@chromium.org, penghuan...@chromium.org, oshima...@chromium.org, wuis...@google.com, wus...@chromium.org, asvitki...@chromium.org, asvitkine...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, jmedle...@chromium.org
Attention needed from Gary Klassen, Ian Vollick and Sirui Wu
Dave Tapuska added 1 comment![]( "Open in Gerrit")
File chrome/browser/glic/glic_selection_observer.cc
Line 684, Patchset 22 (Latest):
if (features::kGlicSelectionPromptUpdatesOnly.Get()) {Dave Tapuska . unresolved
This still does not look correct. This seems to be missing the invoke to show the chip in the side panel for "updates only" feature.
Related details
Attention is currently required from:
- Gary Klassen
- Ian Vollick
- Sirui Wu
Submit Requirements:
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. |
Ian Vollick (Gerrit)
Jul 10, 2026, 1:00:34 PM (yesterday) Jul 10
to Gary Klassen, android-bu...@system.gserviceaccount.com, Sirui Wu, Dave Tapuska, Chromium Metrics Reviews, chromium...@chromium.org, devtools...@chromium.org, net-r...@chromium.org, cblume...@chromium.org, penghuan...@chromium.org, oshima...@chromium.org, wuis...@google.com, wus...@chromium.org, asvitki...@chromium.org, asvitkine...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, jmedle...@chromium.org
Attention needed from Gary Klassen and Sirui Wu
Ian Vollick added 1 comment![]( "Open in Gerrit")
File chrome/browser/glic/browser_ui/glic_selection_widget.cc
Line 235, Patchset 22 (Latest):
const std::u16string& selected_text)Ian Vollick . unresolved
bad rebase?
Related details
Attention is currently required from:
- Gary Klassen
- Sirui Wu
Submit Requirements:
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. |
Gary Klassen (Gerrit)
7:32 AM (7 hours ago) 7:32 AM
to Jerome Jiang, Mirko Bonadei, android-bu...@system.gserviceaccount.com, Ian Vollick, Sirui Wu, Dave Tapuska, Chromium Metrics Reviews, chromium...@chromium.org, eme-r...@chromium.org, jz...@chromium.org, mar...@chromium.org, fgal...@chromium.org, feature-me...@chromium.org, devtools...@chromium.org, net-r...@chromium.org, cblume...@chromium.org, penghuan...@chromium.org, oshima...@chromium.org, wuis...@google.com, wus...@chromium.org, asvitki...@chromium.org, asvitkine...@chromium.org, chrome-intell...@chromium.org, chrome-intelligence-te...@google.com, dewitt...@chromium.org, jmedle...@chromium.org
Attention needed from Dave Tapuska, Ian Vollick and Sirui Wu
Gary Klassen added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 34 (Latest):
Gary Klassen . resolved
Thanks for the review and sorry for the churn and confusion (there were mixed changes from the stack). This has now been cleaned up and should now be better ready for review.
File chrome/browser/glic/browser_ui/glic_selection_widget.cc
Line 235, Patchset 22:
const std::u16string& selected_text)Ian Vollick . resolved
Gary Klassenbad rebase?
Done
File chrome/browser/glic/glic_selection_observer.cc
Line 684, Patchset 22:
if (features::kGlicSelectionPromptUpdatesOnly.Get()) {Dave Tapuska . resolved
This still does not look correct. This seems to be missing the invoke to show the chip in the side panel for "updates only" feature.
Attention is currently required from:
- Dave Tapuska
- Ian Vollick
- Sirui Wu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages