[Customize Toolbar] Pin Chrome Labs by default. [chromium/src : main]
2 views
Skip to first unread message
Taylor Bergquist (Gerrit)
Jul 29, 2024, 5:08:12 PM7/29/24
to Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Taylor Bergquist voted and added 1 comment![]( "Open in Gerrit")
Votes added by Taylor Bergquist
| Commit-Queue | +1 |
1 comment
File chrome/browser/ui/toolbar/pinned_toolbar/pinned_toolbar_actions_model.cc
Line 324, Patchset 1:
actions::ActionManager::Get().FindAction(kActionShowChromeLabs, nullptr);Taylor Bergquist . unresolved
`nullptr` as a scope action here is probably not correct, but the pinned toolbar configuration is not scoped to a browser (nor should it be) so it's not obvious what should go here.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Jul 30, 2024, 11:13:00 AM7/30/24
to Taylor Bergquist, Emily Shack, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Emily Shack and Taylor Bergquist
Caroline Rising added 2 comments![]( "Open in Gerrit")
File chrome/browser/ui/toolbar/pinned_toolbar/pinned_toolbar_actions_model.cc
Line 261, Patchset 4 (Latest):
if (ChromeLabsExists()) {Caroline Rising . unresolved
I think we should be doing this regardless of whether chrome labs exists. If the user resets to default in stable we should probably be changing everything (even chrome labs for canary/dev). What do you think?
Line 269, Patchset 4 (Latest):
const bool labs_is_default = !labs_exists || Contains(kActionShowChromeLabs);Caroline Rising . unresolved
I think this should be in the pref whether the action exists or not. We don't update the profile pref based on whether the action exists (since then we would be unpinning chrome labs when a user opens stable when it should still be pinned for canary)
Related details
Attention is currently required from:
- Emily Shack
- Taylor Bergquist
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taylor Bergquist (Gerrit)
Jul 30, 2024, 2:41:24 PM7/30/24
to Caroline Rising, Emily Shack, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Caroline Rising and Emily Shack
Taylor Bergquist added 3 comments![]( "Open in Gerrit")
File chrome/browser/ui/toolbar/pinned_toolbar/pinned_toolbar_actions_model.cc
I think we should be doing this regardless of whether chrome labs exists. If the user resets to default in stable we should probably be changing everything (even chrome labs for canary/dev). What do you think?
Taylor Bergquist
Oh yeah you're totally right, that makes sense and is also so much simpler!
Done
Line 269, Patchset 4:
const bool labs_is_default = !labs_exists || Contains(kActionShowChromeLabs);Caroline Rising . resolved
I think this should be in the pref whether the action exists or not. We don't update the profile pref based on whether the action exists (since then we would be unpinning chrome labs when a user opens stable when it should still be pinned for canary)
Taylor Bergquist
Done
Line 324, Patchset 1:
actions::ActionManager::Get().FindAction(kActionShowChromeLabs, nullptr);Taylor Bergquist . resolved
`nullptr` as a scope action here is probably not correct, but the pinned toolbar configuration is not scoped to a browser (nor should it be) so it's not obvious what should go here.
Taylor Bergquist
This is irrelevant now again, since we no longer care if labs currently exists or not.
Related details
Attention is currently required from:
- Caroline Rising
- Emily Shack
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taylor Bergquist (Gerrit)
Jul 30, 2024, 2:42:11 PM7/30/24
to Demetrios Papadopoulos, Caroline Rising, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Caroline Rising and Demetrios Papadopoulos
Taylor Bergquist added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Taylor Bergquist . resolved
Demetrios PTAL at the appearance page changes!
Related details
Attention is currently required from:
- Caroline Rising
- Demetrios Papadopoulos
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Jul 30, 2024, 3:09:14 PM7/30/24
to Taylor Bergquist, Caroline Rising, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Caroline Rising and Taylor Bergquist
Demetrios Papadopoulos voted and added 3 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
3 comments
Patchset-level comments
Demetrios Papadopoulos . resolved
LGTM with couple nits.
File chrome/browser/resources/settings/appearance_page/appearance_page.ts
Line 528, Patchset 5 (Latest):
private toolbarPinningStateChanged_(): void {
this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault().then(
isDefault => {
this.showResetPinnedActionsButton_ = !isDefault;
});
}Demetrios Papadopoulos . unresolved
Nit: Leverage async/await
```suggestion
private toolbarPinningStateChanged_() {
this.showResetPinnedActionsButton_ = await !this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault();
}
```
File chrome/test/data/webui/settings/appearance_page_test.ts
Line 365, Patchset 5 (Latest):
});Demetrios Papadopoulos . unresolved
Instead of relying on the default value harcoded in the test proxy, let's explicitly set the value here?
```
appearanceBrowserProxy.setPinnedToolbarActionsAreDefaultResponse(true);
```
Related details
Attention is currently required from:
- Caroline Rising
- Taylor Bergquist
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 30, 2024, 9:18:03 PM7/30/24
to Taylor Bergquist, Demetrios Papadopoulos, Caroline Rising, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Caroline Rising
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention is currently required from:
- Caroline Rising
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Jul 31, 2024, 10:23:59 AM7/31/24
to Taylor Bergquist, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Attention needed from Taylor Bergquist
Caroline Rising voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Taylor Bergquist
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taylor Bergquist (Gerrit)
Jul 31, 2024, 2:33:05 PM7/31/24
to Caroline Rising, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Taylor Bergquist voted and added 2 comments![]( "Open in Gerrit")
Votes added by Taylor Bergquist
| Commit-Queue | +2 |
2 comments
File chrome/browser/resources/settings/appearance_page/appearance_page.ts
Line 528, Patchset 5:
private toolbarPinningStateChanged_(): void {
this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault().then(
isDefault => {
this.showResetPinnedActionsButton_ = !isDefault;
});
}Demetrios Papadopoulos . resolved
Nit: Leverage async/await
```suggestion
private toolbarPinningStateChanged_() {
this.showResetPinnedActionsButton_ = await !this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault();
}
```
Taylor Bergquist
Done
File chrome/test/data/webui/settings/appearance_page_test.ts
Instead of relying on the default value harcoded in the test proxy, let's explicitly set the value here?
```
appearanceBrowserProxy.setPinnedToolbarActionsAreDefaultResponse(true);
```
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 31, 2024, 3:42:03 PM7/31/24
to Taylor Bergquist, Caroline Rising, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resources/settings/appearance_page/appearance_page.ts
Insertions: 3, Deletions: 5.
@@ -525,11 +525,9 @@
return !!this.getPref('autogenerated.theme.policy.color').controlledBy;
}
- private toolbarPinningStateChanged_(): void {
- this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault().then(
- isDefault => {
- this.showResetPinnedActionsButton_ = !isDefault;
- });
+ private async toolbarPinningStateChanged_(): Promise<void> {
+ this.showResetPinnedActionsButton_ =
+ !await this.appearanceBrowserProxy_.pinnedToolbarActionsAreDefault();
}
private isSelectedColorSchemeMode_(colorSchemeMode: ColorSchemeMode):
```
Change information
Commit message:
[Customize Toolbar] Pin Chrome Labs by default.
Moves the reset logic into PinnedToolbarActionsModel for reuse by the
AppearanceHandler and CustomizeToolbarHandler.
Bug: 354690551
Change-Id: I8e81ada3dd9394f489a90deef3bbd6f80972cca3
Commit-Queue: Taylor Bergquist <tberg...@chromium.org>
Reviewed-by: Caroline Rising <cori...@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpa...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1335626}
Files:
- M chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.ts
- M chrome/browser/resources/settings/appearance_page/appearance_page.ts
- M chrome/browser/ui/toolbar/pinned_toolbar/pinned_toolbar_actions_model.cc
- M chrome/browser/ui/toolbar/pinned_toolbar/pinned_toolbar_actions_model.h
- M chrome/browser/ui/webui/settings/appearance_handler.cc
- M chrome/browser/ui/webui/settings/appearance_handler.h
- M chrome/browser/ui/webui/side_panel/customize_chrome/customize_toolbar/customize_toolbar_handler.cc
- M chrome/test/data/webui/settings/appearance_page_test.ts
Change size: M
Delta: 8 files changed, 80 insertions(+), 41 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Caroline Rising, +1 by Demetrios Papadopoulos
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 31, 2024, 3:42:14 PM7/31/24
to Taylor Bergquist, Chromium LUCI CQ, Caroline Rising, Demetrios Papadopoulos, AyeAye, chromium...@chromium.org, blundell+...@chromium.org
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
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