Refactor back/forward button to support WebUI [chromium/src : main]
Paul Jensen (Gerrit)
Paul Jensen added 12 comments![]( "Open in Gerrit")
if (use_webui_back_forward || use_webui_reload) {I think this should be cleaned up by rebasing on https://chromium-review.googlesource.com/c/chromium/src/+/7507149
back_->SetTag(IDC_BACK);
forward_->SetTag(IDC_FORWARD);this logic seems duplicated with the lines on 318 and 322. Can we dedup and move down to line 493 with the other tag setting logic?
if (back_) {do we need the null checks? did the old code handle null controls?
if (id == IDC_FORWARD) {I'd recommend either:
1. removing BackForwardControl::GetTag(), or
2. changing these two if statements to code more like the end of this method like:
```
const std::array<BackForwardControl*, 2> kButtons{back_, forward_};
auto it = std::ranges::find(kButtons, id, &BackForwardControl::GetTag);
if (it != kButtons.end()) {
(*it)->SetEnabled(enabled);
}
```
Back(array<ClickDispositionFlag> flags);Paul JensenFYI we're moving away from ClickDispositionFlag as per https://chromium-review.googlesource.com/c/chromium/src/+/7405167/10/components/browser_apis/browser_controls/browser_controls_api.mojom#42
We wound up moving back to ClickDispositionFlag.
return features::IsWebUIReloadButtonEnabled() ||I think this will get cleaned up with https://chromium-review.googlesource.com/c/chromium/src/+/7507149
// When enable, the back and forward buttons will be replaced with the alooks like folks generally don't add comments in this file
// UI reload behavior. If the renderer crashes, we will try to recover it bybehavior behavior seems like a typo
// When enabled, the back and forward buttons will be replaced with the asame comment as https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/a5aa39cc_648d0834/ and the following line's comment
SetBackButtonLeadingMargin(int32 margin);```suggestion
OnBackButtonLeadingMarginChanged(int32 margin);
```
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 13 comments![]( "Open in Gerrit")
private getDisposition_(e: MouseEvent): WindowOpenDisposition {move back to click disposition
I think this should be cleaned up by rebasing on https://chromium-review.googlesource.com/c/chromium/src/+/7507149
Acknowledged
back_->SetTag(IDC_BACK);
forward_->SetTag(IDC_FORWARD);this logic seems duplicated with the lines on 318 and 322. Can we dedup and move down to line 493 with the other tag setting logic?
Made a helper to combine logic.
do we need the null checks? did the old code handle null controls?
Removed.
I'd recommend either:
1. removing BackForwardControl::GetTag(), or
2. changing these two if statements to code more like the end of this method like:
```
const std::array<BackForwardControl*, 2> kButtons{back_, forward_};
auto it = std::ranges::find(kButtons, id, &BackForwardControl::GetTag);
if (it != kButtons.end()) {
(*it)->SetEnabled(enabled);
}
```
Done
Back(array<ClickDispositionFlag> flags);FYI we're moving away from ClickDispositionFlag as per https://chromium-review.googlesource.com/c/chromium/src/+/7405167/10/components/browser_apis/browser_controls/browser_controls_api.mojom#42
Acknowledged
I think this will get cleaned up with https://chromium-review.googlesource.com/c/chromium/src/+/7507149
Done
// When enable, the back and forward buttons will be replaced with the alooks like folks generally don't add comments in this file
Done
// UI reload behavior. If the renderer crashes, we will try to recover it bybehavior behavior seems like a typo
Done
// When enabled, the back and forward buttons will be replaced with the asame comment as https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/a5aa39cc_648d0834/ and the following line's comment
Done
SetBackButtonLeadingMargin(int32 margin);Youseff Bourouphel```suggestion
OnBackButtonLeadingMarginChanged(int32 margin);
```
Done
Related details
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 5 comments![]( "Open in Gerrit")
some quick comments. Will take a look later today.
bool IsWebUIBackForwardButtonEnabled();probably a good idea to just pull all the feature flag changes to a separate CL, like https://crrev.com/c/7523686
void ToolbarView::EnabledStateChangedForCommand(int id, bool enabled) {I submitted a CL that fixed a bug in this function. After rebasing, I think you don't need any change to this function.
BASE_FEATURE(kWebUIBackForwardButton, base::FEATURE_DISABLED_BY_DEFAULT);add a comment for it
Related details
- Paul Jensen
- Youseff Bourouphel
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. |
Paul Jensen (Gerrit)
Paul Jensen added 11 comments![]( "Open in Gerrit")
display: flex;what's the goal of this change?
protected accessor backVisible_: boolean = true;do we need this one?
private getFlags_(e: MouseEvent): ClickDispositionFlag[] {can we share this method with the ReloadButton's generateFlags() maybe in a `toolbar_button.ts`?
// Copyright 2026 The Chromium Authorsperhaps make this the second CL
back->AsView() ? back->AsView()->GetID() : VIEW_ID_BACK_BUTTON;Can we add a BackForwardControl::GetID() method and change this whole method to be a for-loop that does the same thing for both buttons? If it's small, then we could remove this method and put the new for-loop right before the existing for-loop.
if (use_webui_back_forward || use_webui_reload) {I think this should get cleaned up if you fix the merge conflicts
// Copyright 2026 The Chromium Authorsthis could be third CL
COMPONENT_EXPORT(CHROME_FEATURES)this would be first CL
OnBackForwardStatusChanged(BackForwardState state);```suggestion
OnBackForwardStatusChanged(ButtonState back_state, ButtonState forward_state);
```
struct BackForwardState {maybe change this to just ButtonState, also see what Qingxin's CL uses, maybe combine with that
.icon-arrow-forward {could we use the back icon flipped?
Related details
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 5 comments![]( "Open in Gerrit")
<back-forward-button id="back" direction="back"In my CL, I was dynamically calculating the webview's size based on how many webui buttons are enabled. If we always have the buttons (same to the existing reload button as well) in the html, then if this feature flag is disabled and reload button is enabled, this back-forward button will take the one-button sized webview, and there'll be no place for reload button.
private contextMenuPosition() {we have this function for all buttons. I'd definitely like to create a shared function for all of them. I'll work on it.
if (use_webui_back_forward || use_webui_reload) {I think this should get cleaned up if you fix the merge conflicts
features::IsWebUIToolbarEnabled(), see https://crsrc.org/c/chrome/browser/ui/views/toolbar/toolbar_view.cc;drc=96dad451ae9bfef46c85c0b5a0cd4025e13af109;l=318.
And update IsWebUIToolbarEnabled to check your feature flag as well, like https://chromium-review.googlesource.com/c/chromium/src/+/7523686 as I suggested in another comment.
back_ = toolbar_webview_->GetBackControl();why do we want to set back_ when we use webui back button?
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
return;I think it's due to you set back_ to the webui back button in toolbar_view.cc. If we don't set it there, then we should be fine.
Related details
- Youseff Bourouphel
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. |
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
back_ = toolbar_webview_->GetBackControl();why do we want to set back_ when we use webui back button?
in `toolbar_view.h` this CL changes the type of `back_` to be a parent class that both the WebUI and Views code implement. This allows existing code dereferencing `back_` to continue working (assuming the new parent class implements the desired API being used).
Related details
- Qingxin Wu
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
private contextMenuPosition() {we have this function for all buttons. I'd definitely like to create a shared function for all of them. I'll work on it.
https://crrev.com/c/7528033. It only moved this function since my typescript also uses this function, and you can move other functions to that file as well if you need.
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 18 comments![]( "Open in Gerrit")
what's the goal of this change?
This change is necessary because the toolbar-app component now contains multiple child elements: the back button, the forward button, and the reload button
In my CL, I was dynamically calculating the webview's size based on how many webui buttons are enabled. If we always have the buttons (same to the existing reload button as well) in the html, then if this feature flag is disabled and reload button is enabled, this back-forward button will take the one-button sized webview, and there'll be no place for reload button.
good call! Done.
do we need this one?
Yes. Done
private getFlags_(e: MouseEvent): ClickDispositionFlag[] {can we share this method with the ReloadButton's generateFlags() maybe in a `toolbar_button.ts`?
Done. Moved to `chrome/browser/resources/webui_toolbar/toolbar_button.ts`.
Qingxin Wuwe have this function for all buttons. I'd definitely like to create a shared function for all of them. I'll work on it.
https://crrev.com/c/7528033. It only moved this function since my typescript also uses this function, and you can move other functions to that file as well if you need.
SGTM. I will build on top of that.
add back new line at end of file.
Done
probably a good idea to just pull all the feature flag changes to a separate CL, like https://crrev.com/c/7523686
Acknowledged
perhaps make this the second CL
Acknowledged
back->AsView() ? back->AsView()->GetID() : VIEW_ID_BACK_BUTTON;Can we add a BackForwardControl::GetID() method and change this whole method to be a for-loop that does the same thing for both buttons? If it's small, then we could remove this method and put the new for-loop right before the existing for-loop.
Done
Qingxin WuI think this should get cleaned up if you fix the merge conflicts
features::IsWebUIToolbarEnabled(), see https://crsrc.org/c/chrome/browser/ui/views/toolbar/toolbar_view.cc;drc=96dad451ae9bfef46c85c0b5a0cd4025e13af109;l=318.
And update IsWebUIToolbarEnabled to check your feature flag as well, like https://chromium-review.googlesource.com/c/chromium/src/+/7523686 as I suggested in another comment.
Done
void ToolbarView::EnabledStateChangedForCommand(int id, bool enabled) {I submitted a CL that fixed a bug in this function. After rebasing, I think you don't need any change to this function.
The issue is that `back_` and `forward_` are now typed as `BackForwardControl*` to support both the native and WebUI implementations, whereas `reload_`, `home_`, and `avatar_` are still `views::Button*`.
Since BackForwardControl is an interface that does not inherit from views::Button, I cannot put them all into a single `std::array<views::Button*>` anymore. I have to handle the BackForwardControls and the standard views::Buttons separately.
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
return;I think it's due to you set back_ to the webui back button in toolbar_view.cc. If we don't set it there, then we should be fine.
resolving as I think this was answered in Paul's response to another comment.
this could be third CL
Acknowledged
this would be first CL
Acknowledged
BASE_FEATURE(kWebUIBackForwardButton, base::FEATURE_DISABLED_BY_DEFAULT);add a comment for it
Done
```suggestion
OnBackForwardStatusChanged(ButtonState back_state, ButtonState forward_state);
```
Done
maybe change this to just ButtonState, also see what Qingxin's CL uses, maybe combine with that
Done
could we use the back icon flipped?
- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
class="${This button also has accessibility fields like aria-label and aria-haspopup?
https://crsrc.org/c/chrome/browser/ui/views/toolbar/back_forward_button.cc;drc=57b039617d6f75845e77204b61838efd868be036;l=45
We'll define them in the typescript side for webui toobars, e.g.,:
https://crsrc.org/c/chrome/browser/resources/webui_toolbar/reload_button.html.ts;drc=c24c50287e9d03c5b6e9abc1c9a7ba0183c60bf4;l=13
(just noticed I forgot to send this comment out..will take another look today. Got a ton of comments in my CL as well)
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
class="${This button also has accessibility fields like aria-label and aria-haspopup?
https://crsrc.org/c/chrome/browser/ui/views/toolbar/back_forward_button.cc;drc=57b039617d6f75845e77204b61838efd868be036;l=45
We'll define them in the typescript side for webui toobars, e.g.,:
https://crsrc.org/c/chrome/browser/resources/webui_toolbar/reload_button.html.ts;drc=c24c50287e9d03c5b6e9abc1c9a7ba0183c60bf4;l=13
(just noticed I forgot to send this comment out..will take another look today. Got a ton of comments in my CL as well)
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 5 comments![]( "Open in Gerrit")
display: flex;Youseff Bourouphelwhat's the goal of this change?
This change is necessary because the toolbar-app component now contains multiple child elements: the back button, the forward button, and the reload button
Why does it need to flex? and why do we need to align in the center?
auto back = std::make_unique<BackForwardButton>(
BackForwardButton::Direction::kBack,
base::BindRepeating(callback, browser_, IDC_BACK), browser_);
auto forward = std::make_unique<BackForwardButton>(
BackForwardButton::Direction::kForward,
base::BindRepeating(callback, browser_, IDC_FORWARD), browser_);
back_ = AddChildView(std::move(back));
forward_ = AddChildView(std::move(forward));
if (features::IsWebUIReloadButtonEnabled() ||
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
auto toolbar_webview = std::make_unique<WebUIToolbarWebView>(
browser_, browser_->command_controller());
toolbar_webview_ = AddChildView(std::move(toolbar_webview));can we undo changes to these lines? they look non-functional
back_->SetVisible(false);
forward_->SetVisible(false);can we instead not create the BackForwardButton's rather than create them and hide them?
#include "chrome/browser/ui/views/toolbar/back_forward_control.h"can we replace this with "class BackForwardControl;"?
if (features::IsWebUIReloadButtonEnabled()) {can we revert this back to the previous code?
Related details
- Qingxin Wu
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Message from Qingxin Wu![]( "Open in Gerrit")
Overall, this CL looks good and follows the patterns we established for the Split Tabs button. However, there are a few important details to address, particularly around initial state population (to avoid flicker) and accessibility.
Similar to our work on Split Tabs (@7458181), please consider the following:
- *1. Initial State Population (Flicker Prevention):**
- In `webui_toolbar_ui.cc`, you are only adding the feature flag `backForwardButtonEnabled` to the data source. You are **not** adding the initial `enabled` state of the back/forward buttons.
- In `app.ts`, `backEnabled_` and `forwardEnabled_` default to `false`.
- This means when the toolbar loads, the buttons will be disabled until the first Mojo update arrives. Since `BrowserControlsService::OnPageInitialized` (in this CL) doesn't explicitly push the current state, the buttons might remain disabled until the user navigates or something triggers an update.
- **Suggestion:** In `WebUIToolbarUI::WebUIToolbarUI`, populate the initial `backEnabled` and `forwardEnabled` values in the data source (using `chrome::IsCommandEnabled(browser, IDC_BACK/FORWARD)`), similar to how we populated `initialIsSplit` for Split Tabs. Alternatively, ensure `WebUIToolbarWebView::OnPageInitialized` triggers a state push.
- *2. Accessibility Labels:**
- In `back_forward_button.html.ts`, the `<cr-icon-button>` elements are missing accessibility labels (`aria-label` or `title`).
- **Suggestion:** Add `title="${this.label_}"` (or similar) to the template. You can fetch the localized strings (which you correctly added to `webui_toolbar_ui.cc`) in `back_forward_button.ts` using `loadTimeData`, just like we did for Split Tabs.
- *3. Middle Click Handling:**
- In `back_forward_button.ts`, `onClick_` handles `e.button === 1` (middle click). However, standard `click` events usually only fire for the primary button (left click). Middle clicks often fire `auxclick`.
- **Suggestion:** Verify if `<cr-icon-button>` fires `click` for middle clicks. If not, you may need to listen to `@auxclick` in `back_forward_button.html.ts` to support opening in a new tab via middle-click.
**4. Integration with Split Tabs:**
Please ensure `BrowserControlsService::OnPageInitialized` preserves the Split Tabs initialization logic (calling `IsButtonPinned` / `OnButtonPinStateChanged` and `OnTabSplitStatusChanged`) which was recently added. The diff here shows a clean `OnPageInitialized`, which might conflict with recent changes.
**5. Testing:**
Great job adding comprehensive tests in `WebUIToolbarWebViewBackForwardBrowserTest`. The use of Shadow DOM traversal in `EvalJs` looks correct and robust.
Related details
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 2 comments![]( "Open in Gerrit")
oh ignore my last comment which was auto posted by gemini. Sorry about that.
static override get properties() {There seems to be an ordering issue in this file. Can we re-order following WebUI conventions as suggested in this comment in another CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/b6d8c5b6_fd4fd453/
Qingxin Wu (Gerrit)
Qingxin Wu added 6 comments![]( "Open in Gerrit")
${
this.backForwardButtonEnabled_ ? html`
<back-forward-button id="back" direction="back"
aria-label="$i18n{backButtonAccName}"
title="$i18n{backButtonTooltip}"
?disabled="${!this.backEnabled_}"
.leadingMargin="${this.backButtonMargin_}"></back-forward-button>
<back-forward-button id="forward" direction="forward"
aria-label="$i18n{forwardButtonAccName}"
title="$i18n{forwardButtonTooltip}"
?disabled="${!this.forwardEnabled_}"
?hidden="${!this.forwardVisible_}"></back-forward-button>
` :
''}
<reload-button-app id="reload"></reload-button-app>My CL got the same comment [0] below (you can see how I reformatted it):
keep these on the same line. The first line of the template should also have 0 indent.
Use // clang-format off and // clang-format on comments before and after the template string to prevent clang format from messing it up. Clang format doesn't really know how to format HTML strings in TS files.
[0] https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/d1986a86_c0088c8f/
:host([hidden]) {
display: none;
}import from cr_hidden_style_lit instead?
See split_tabs_button.css https://chromium-review.googlesource.com/c/chromium/src/+/7458181/43..44
override render() {
return getHtml.bind(this)();
}move this up to the top of the file, right after get styles()
WindowOpenDisposition,where is this used in other files? if not used, we don't need to export it?
if (features::IsWebUIReloadButtonEnabled() ||
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {Probably a rebase mistake. Instead of this, change this back to features::IsWebUIToolbarEnabled(), but update features::IsWebUIToolbarEnabled() to consider kWebUIBackForwardButton as well.
// Handle back and forward buttons.
const std::array<BackForwardControl*, 2> back_forward_controls{back_,
forward_};
auto bf_it = std::ranges::find_if(
back_forward_controls,
[id](BackForwardControl* b) { return b && b->GetTag() == id; });
if (bf_it != back_forward_controls.end()) {
(*bf_it)->SetEnabled(enabled);
return;
}why do we need to have these two separately?
Qingxin Wu (Gerrit)
Qingxin Wu added 2 comments![]( "Open in Gerrit")
protected accessor backForwardButtonEnabled_: boolean =I'm probably missing something here, but wondering why we needed these properties in this file, other than back_forward_button.ts.
this.browserProxy_.callbackRouter.onBackForwardStatusChanged.addListener(I think we should add the listeners in connectedCallback() instead of the constructor, and remove the listeners in disconnectedCallback(), otherwise there'll be memory leaks. But let me know if I missed something here.
See discussion in another CL's comment: https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/9a49a6c0_ea7d425f/
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
this.browserProxy_.callbackRouter.onBackButtonLeadingMarginChangeddo we need separate mojo functions to get the initial state of the back forward buttons? See https://crrev.com/c/7535796 and https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/ for the conversation on my CL for more context.
Qingxin Wu (Gerrit)
Qingxin Wu added 2 comments![]( "Open in Gerrit")
display: flex;Youseff Bourouphelwhat's the goal of this change?
Paul JensenThis change is necessary because the toolbar-app component now contains multiple child elements: the back button, the forward button, and the reload button
Qingxin WuWhy does it need to flex? and why do we need to align in the center?
the host element is a custom element (which defaults to display: inline), we set `display: flex` to enforce a block-level flex container. This ensures the child buttons render in a row natually and allows us to use gap for precise spacing between elements inside. It's annoying to manage the elements inside and handle the gap (probably through margin) if it was inline. I also saw the button rendered misaligned when I was not using flex.
// Notifies that the back button was hovered.nit: back or forward?
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 16 comments![]( "Open in Gerrit")
display: flex;Youseff Bourouphelwhat's the goal of this change?
Paul JensenThis change is necessary because the toolbar-app component now contains multiple child elements: the back button, the forward button, and the reload button
Why does it need to flex? and why do we need to align in the center?
The flex is important to ensure it being a row of items. I figured we would want it centered in the box but after testing i see it makes no difference. I removed that.
this.backForwardButtonEnabled_ ? html`
<back-forward-button id="back" direction="back"
aria-label="$i18n{backButtonAccName}"
title="$i18n{backButtonTooltip}"
?disabled="${!this.backEnabled_}"
.leadingMargin="${this.backButtonMargin_}"></back-forward-button>
<back-forward-button id="forward" direction="forward"
aria-label="$i18n{forwardButtonAccName}"
title="$i18n{forwardButtonTooltip}"
?disabled="${!this.forwardEnabled_}"
?hidden="${!this.forwardVisible_}"></back-forward-button>
` :
''}
<reload-button-app id="reload"></reload-button-app>My CL got the same comment [0] below (you can see how I reformatted it):
keep these on the same line. The first line of the template should also have 0 indent.Use // clang-format off and // clang-format on comments before and after the template string to prevent clang format from messing it up. Clang format doesn't really know how to format HTML strings in TS files.
[0] https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/d1986a86_c0088c8f/
Done
static override get properties() {There seems to be an ordering issue in this file. Can we re-order following WebUI conventions as suggested in this comment in another CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/b6d8c5b6_fd4fd453/
Acknowledged
I'm probably missing something here, but wondering why we needed these properties in this file, other than back_forward_button.ts.
Yeah, in an effort to seperate out all of the back/forward button logic, I made `back_forward_section` which will hold the two buttons and manage the logic that controls them.
This should improve separation of concerns and keeps the codebase cleaner by decoupling that logic from the main view.
this.browserProxy_.callbackRouter.onBackForwardStatusChanged.addListener(I think we should add the listeners in connectedCallback() instead of the constructor, and remove the listeners in disconnectedCallback(), otherwise there'll be memory leaks. But let me know if I missed something here.
See discussion in another CL's comment: https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/9a49a6c0_ea7d425f/
Acknowledged
this.browserProxy_.callbackRouter.onBackButtonLeadingMarginChangeddo we need separate mojo functions to get the initial state of the back forward buttons? See https://crrev.com/c/7535796 and https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/ for the conversation on my CL for more context.
I've updated the implementation to follow the 'Push on Subscribe' pattern recommended in that conversation.
Instead of adding separate Get Mojo functions for the initial state, I modified BrowserControlsService::AddObserver to immediately query the delegate and push the current button state and margin to the observer. This ensures the WebUI receives the correct initial state as soon as it subscribes, reusing the existing OnBackForwardStatusChanged and OnBackButtonLeadingMarginChanged listeners.
Let me know if I should be doing it differently!
import from cr_hidden_style_lit instead?
See split_tabs_button.css https://chromium-review.googlesource.com/c/chromium/src/+/7458181/43..44
Done
move this up to the top of the file, right after get styles()
Done
where is this used in other files? if not used, we don't need to export it?
Acknowledged
if (features::IsWebUIReloadButtonEnabled() ||
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {Probably a rebase mistake. Instead of this, change this back to features::IsWebUIToolbarEnabled(), but update features::IsWebUIToolbarEnabled() to consider kWebUIBackForwardButton as well.
Acknowledged
auto back = std::make_unique<BackForwardButton>(
BackForwardButton::Direction::kBack,
base::BindRepeating(callback, browser_, IDC_BACK), browser_);
auto forward = std::make_unique<BackForwardButton>(
BackForwardButton::Direction::kForward,
base::BindRepeating(callback, browser_, IDC_FORWARD), browser_);
back_ = AddChildView(std::move(back));
forward_ = AddChildView(std::move(forward));
if (features::IsWebUIReloadButtonEnabled() ||
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
auto toolbar_webview = std::make_unique<WebUIToolbarWebView>(
browser_, browser_->command_controller());
toolbar_webview_ = AddChildView(std::move(toolbar_webview));can we undo changes to these lines? they look non-functional
Acknowledged
can we instead not create the BackForwardButton's rather than create them and hide them?
Done
// Handle back and forward buttons.
const std::array<BackForwardControl*, 2> back_forward_controls{back_,
forward_};
auto bf_it = std::ranges::find_if(
back_forward_controls,
[id](BackForwardControl* b) { return b && b->GetTag() == id; });
if (bf_it != back_forward_controls.end()) {
(*bf_it)->SetEnabled(enabled);
return;
}Youseff Bourouphelwhy do we need to have these two separately?
BackForwardControl is an abstract interface and not a views::View, which allows back_/forward_ to point to either native or WebUI implementations. Because of this type difference, they cannot be added to the std::array<views::Button*> used for the other buttons (like reload_, which is typed concretely as a ReloadButton view).
#include "chrome/browser/ui/views/toolbar/back_forward_control.h"can we replace this with "class BackForwardControl;"?
I don't think we can replace it. The test calls `GetVisible()` on the `BackForwardControl*` returned by `GetForwardButton()`. Since we are accessing a member function, the compiler needs the full class definition from the header file.
can we revert this back to the previous code?
Done
// Notifies that the back button was hovered.- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
It looks like https://chromium-review.googlesource.com/c/chromium/src/+/7458181 is going to land shortly, can we rebase this CL on top of that CL? I think it will make it easier to review by having the new code along side the split tabs button changes, and it will remove some of the duplicated changes.
Related details
- Qingxin Wu
- Youseff Bourouphel
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. |
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
this.backForwardStatusChangedListenerId_ =rather than move this into BackForwardSectionElement, can we instead move it into back_forward_button.ts (and eventually it will move into toolbar_button.ts)? basically each button can listen for changes to it's button type.
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
this.browserProxy_.callbackRouter.onBackButtonLeadingMarginChangedYouseff Bouroupheldo we need separate mojo functions to get the initial state of the back forward buttons? See https://crrev.com/c/7535796 and https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/ for the conversation on my CL for more context.
I've updated the implementation to follow the 'Push on Subscribe' pattern recommended in that conversation.
Instead of adding separate Get Mojo functions for the initial state, I modified BrowserControlsService::AddObserver to immediately query the delegate and push the current button state and margin to the observer. This ensures the WebUI receives the correct initial state as soon as it subscribes, reusing the existing OnBackForwardStatusChanged and OnBackButtonLeadingMarginChanged listeners.
Let me know if I should be doing it differently!
no, we were suggested to: In connectedCallback
- Get initial state
- Add listener to receive future updates.
sending a magical initial event the first time a listener is added is an anti-pattern for the rest of WebUI code. Instead explicit query and observe APIs are preferred. If you look at the most recent discussions on https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/, you'll see that we changed to use the pull model.
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 3 comments![]( "Open in Gerrit")
It looks like https://chromium-review.googlesource.com/c/chromium/src/+/7458181 is going to land shortly, can we rebase this CL on top of that CL? I think it will make it easier to review by having the new code along side the split tabs button changes, and it will remove some of the duplicated changes.
Done
this.browserProxy_.callbackRouter.onBackButtonLeadingMarginChangedYouseff Bouroupheldo we need separate mojo functions to get the initial state of the back forward buttons? See https://crrev.com/c/7535796 and https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/ for the conversation on my CL for more context.
Qingxin WuI've updated the implementation to follow the 'Push on Subscribe' pattern recommended in that conversation.
Instead of adding separate Get Mojo functions for the initial state, I modified BrowserControlsService::AddObserver to immediately query the delegate and push the current button state and margin to the observer. This ensures the WebUI receives the correct initial state as soon as it subscribes, reusing the existing OnBackForwardStatusChanged and OnBackButtonLeadingMarginChanged listeners.
Let me know if I should be doing it differently!
no, we were suggested to: In connectedCallback
- Get initial state
- Add listener to receive future updates.
sending a magical initial event the first time a listener is added is an anti-pattern for the rest of WebUI code. Instead explicit query and observe APIs are preferred. If you look at the most recent discussions on https://chromium-review.googlesource.com/c/chromium/src/+/7458181/comment/baed1082_299042e6/, you'll see that we changed to use the pull model.
Thanks for clarifying I just did that. I also moved the logic under chrome/browser/resources/webui_toolbar/back_forward_button.ts as per Paul's comment here https://chromium-review.googlesource.com/c/chromium/src/+/7466673/comment/ea7a5022_7028bad3/
rather than move this into BackForwardSectionElement, can we instead move it into back_forward_button.ts (and eventually it will move into toolbar_button.ts)? basically each button can listen for changes to it's button type.
- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 18 comments![]( "Open in Gerrit")
${this.backForwardButtonEnabled_ ? html`ditto
```suggestion
${this.backForwardButtonEnabled_ ? html`
```
aria-label="$i18n{backButtonAccName}"
title="$i18n{backButtonTooltip}"></back-forward-button>
<back-forward-button id="forward" direction="forward"
aria-label="$i18n{forwardButtonAccName}"
title="$i18n{forwardButtonTooltip}"></back-forward-button>` : ''}Hmm, should we move the aria-label and title pieces into the buttons themselves? i.e. in the back_forward_button.html.ts you'd have
```
aria-label="$i18n{this.direction === "back" ? backButonAccName : forwardButtonAccName}"
```
backForwardButtonEnabled_: {type: Boolean},for uniformity with above flags
```suggestion
isBackForwardButtonEnabled_: {type: Boolean},
```
protected accessor backForwardButtonEnabled_: boolean =ditto
```suggestion
protected accessor isBackForwardButtonEnabled_: boolean =
```
loadTimeData.getBoolean('backForwardButtonEnabled');ditto
```suggestion
loadTimeData.getBoolean('enableBackForwardButtons');
```
height: 34px;more like split_tabs_button.css:
```suggestion
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;
height: var(--cr-icon-button-size);
width: var(--cr-icon-button-size);
```
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;Then we can remove these lines
```suggestion
```
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
// Back button is always visible.
button_count++;
if (forward_control_.GetVisible()) {
button_count++;
}
}to be more like above code
```suggestion
button_count += base::FeatureList::IsEnabled(features::kWebUIBackForwardButton);
button_count += base::FeatureList::IsEnabled(features::kWebUIBackForwardButton) &&
forward_control_.GetVisible();
```
WebUIBackForwardControl* WebUIToolbarWebView::GetBackControl() {
return &back_control_;
}
WebUIBackForwardControl* WebUIToolbarWebView::GetForwardControl() {
return &forward_control_;
}can we move these up so they're next to GetReloadControl()?
// MetricsReporterService lifetime is tied to `web_contents_` which can bewhy are we removing the comma?
void SetBackForwardButtonState(bool back_enabled,
bool back_visible,
bool forward_enabled,
bool forward_visible);```suggestion
void SetBackForwardButtonState(browser_controls_api::mojom::ButtonState back_state,
browser_controls_api::mojom::ButtonState forward_state);
```
virtual void GetBackForwardButtonStates(bool* back_enabled,
bool* back_visible,
bool* forward_enabled,
bool* forward_visible) = 0;```suggestion
virtual void GetBackForwardButtonStates(browser_controls_api::mojom::ButtonStatePtr back_state,
browser_controls_api::mojom::ButtonStatePtr forward_state) = 0;
```
// Gets the argument time immediately after executing the command.why this change?
void SetBackForwardButtonState(bool back_enabled,
bool back_visible,
bool forward_enabled,
bool forward_visible);```suggestion
void SetBackForwardButtonState(browser_controls_api::mojom::ButtonState back_state,
browser_controls_api::mojom::ButtonState forward_state);
```
source->AddBoolean(
"backForwardButtonEnabled",
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton));can we move this down beside the enableReloadButton code below?
source->AddBoolean("backEnabled",
chrome::IsCommandEnabled(browser, IDC_BACK));
source->AddBoolean("forwardEnabled",
chrome::IsCommandEnabled(browser, IDC_FORWARD));I'm told LoadTimeData should not be used for things that are not constant across a browser session (i.e. basically should only be used for feature flags and localized strings). Can we get rid of this code and rely on the getBackForwardState() call in the back_forward_button.ts constructor?
// Notifies that the back button was hovered.This comment says back button only, but the API takes a control type. Perhaps we should change the API to just be `BackButtonHovered()`?
transform: scaleX(-1);Can we move this into back_forward_button.css? e.g. hardcode the icon as the forward one but then add some conditional CSS like:
```
blah[direction=back] {
transform: scaleX(-1);
}
```
Related details
- Qingxin Wu
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 6 comments![]( "Open in Gerrit")
--cr-icon-button-size: 34px;I guess your buttons would have the same touch mode issue, where the icon would be larger in touch mode. I'm trying to fix it for split tabs button. You can probably fix it after my WIP CL (https://crrev.com/c/7533005) is proved working. Just a heads up.
menu_model_.get(), views::MenuRunner::CONTEXT_MENU);should this be `views::MenuRunner::HAS_MNEMONICS`?
From my understanding, BackForwardButton relies on ToolbarButton::ShowContextMenuForViewImpl (for right-click) or ToolbarButton::OnMousePressed (for long-press), which both call ShowDropDownMenu. ShowDropDownMenu calls `ShowMenuForModel` [0], which creates its MenuRunner with HAS_MNEMONICS. I was fixing the context menu for split tabs button's right click as well, but let me know if I missunderstood how the Views buttons work.
bool did_recover_from_previous_termination_ = false;seems to be refactor issue. This has been removed in https://chromium-review.googlesource.com/c/chromium/src/+/7541177
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {we should use features::IsWebUIBackForwardButtonEnabled? same below
if (forward_control_.GetVisible()) {
button_count++;
}nit: `button_count += forward_control_.GetVisible();`
// Gets the argument time immediately after executing the command.- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 28 comments![]( "Open in Gerrit")
ditto
```suggestion
${this.backForwardButtonEnabled_ ? html`
```
Done
ditto
```suggestion
${this.backForwardButtonEnabled_ ? html`
```
Done
aria-label="$i18n{backButtonAccName}"
title="$i18n{backButtonTooltip}"></back-forward-button>
<back-forward-button id="forward" direction="forward"
aria-label="$i18n{forwardButtonAccName}"
title="$i18n{forwardButtonTooltip}"></back-forward-button>` : ''}Hmm, should we move the aria-label and title pieces into the buttons themselves? i.e. in the back_forward_button.html.ts you'd have
```
aria-label="$i18n{this.direction === "back" ? backButonAccName : forwardButtonAccName}"
```
Done
aria-label="$i18n{backButtonAccName}"
title="$i18n{backButtonTooltip}"></back-forward-button>
<back-forward-button id="forward" direction="forward"
aria-label="$i18n{forwardButtonAccName}"
title="$i18n{forwardButtonTooltip}"></back-forward-button>` : ''}Hmm, should we move the aria-label and title pieces into the buttons themselves? i.e. in the back_forward_button.html.ts you'd have
```
aria-label="$i18n{this.direction === "back" ? backButonAccName : forwardButtonAccName}"
```
Done
for uniformity with above flags
```suggestion
isBackForwardButtonEnabled_: {type: Boolean},
```
Done
ditto
```suggestion
protected accessor isBackForwardButtonEnabled_: boolean =
```
Done
ditto
```suggestion
loadTimeData.getBoolean('enableBackForwardButtons');
```
Done
more like split_tabs_button.css:
```suggestion
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;
height: var(--cr-icon-button-size);
width: var(--cr-icon-button-size);
```
Done
more like split_tabs_button.css:
```suggestion
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;
height: var(--cr-icon-button-size);
width: var(--cr-icon-button-size);
```
Done
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;Then we can remove these lines
```suggestion
```
moved up into :host just like split tabs
I guess your buttons would have the same touch mode issue, where the icon would be larger in touch mode. I'm trying to fix it for split tabs button. You can probably fix it after my WIP CL (https://crrev.com/c/7533005) is proved working. Just a heads up.
Thanks! Will make the fix when I see your CL is working.
--cr-icon-button-icon-size: 20px;
--cr-icon-button-margin-end: 0;
--cr-icon-button-margin-start: 0;
--cr-icon-button-size: 34px;Then we can remove these lines
```suggestion
```
Done
should this be `views::MenuRunner::HAS_MNEMONICS`?
From my understanding, BackForwardButton relies on ToolbarButton::ShowContextMenuForViewImpl (for right-click) or ToolbarButton::OnMousePressed (for long-press), which both call ShowDropDownMenu. ShowDropDownMenu calls `ShowMenuForModel` [0], which creates its MenuRunner with HAS_MNEMONICS. I was fixing the context menu for split tabs button's right click as well, but let me know if I missunderstood how the Views buttons work.
interesting, thanks for pointing this out!
seems to be refactor issue. This has been removed in https://chromium-review.googlesource.com/c/chromium/src/+/7541177
Done
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {we should use features::IsWebUIBackForwardButtonEnabled? same below
Done
if (forward_control_.GetVisible()) {
button_count++;
}Youseff Bourouphelnit: `button_count += forward_control_.GetVisible();`
Done
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {
// Back button is always visible.
button_count++;
if (forward_control_.GetVisible()) {
button_count++;
}
}to be more like above code
```suggestion
button_count += base::FeatureList::IsEnabled(features::kWebUIBackForwardButton);
button_count += base::FeatureList::IsEnabled(features::kWebUIBackForwardButton) &&
forward_control_.GetVisible();
```
Done
WebUIBackForwardControl* WebUIToolbarWebView::GetBackControl() {
return &back_control_;
}
WebUIBackForwardControl* WebUIToolbarWebView::GetForwardControl() {
return &forward_control_;
}can we move these up so they're next to GetReloadControl()?
Done
// MetricsReporterService lifetime is tied to `web_contents_` which can bewhy are we removing the comma?
Mistake. Reverted
void SetBackForwardButtonState(bool back_enabled,
bool back_visible,
bool forward_enabled,
bool forward_visible);```suggestion
void SetBackForwardButtonState(browser_controls_api::mojom::ButtonState back_state,
browser_controls_api::mojom::ButtonState forward_state);
```
Done
virtual void GetBackForwardButtonStates(bool* back_enabled,
bool* back_visible,
bool* forward_enabled,
bool* forward_visible) = 0;```suggestion
virtual void GetBackForwardButtonStates(browser_controls_api::mojom::ButtonStatePtr back_state,
browser_controls_api::mojom::ButtonStatePtr forward_state) = 0;
```
Done
// Gets the argument time immediately after executing the command.Youseff Bourouphelwhy this change?
Mistake. Reverted.
// Gets the argument time immediately after executing the command.what does this "argument time" mean
mistake, i reverted.
void SetBackForwardButtonState(bool back_enabled,
bool back_visible,
bool forward_enabled,
bool forward_visible);```suggestion
void SetBackForwardButtonState(browser_controls_api::mojom::ButtonState back_state,
browser_controls_api::mojom::ButtonState forward_state);
```
Done
source->AddBoolean(
"backForwardButtonEnabled",
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton));can we move this down beside the enableReloadButton code below?
Done
source->AddBoolean("backEnabled",
chrome::IsCommandEnabled(browser, IDC_BACK));
source->AddBoolean("forwardEnabled",
chrome::IsCommandEnabled(browser, IDC_FORWARD));I'm told LoadTimeData should not be used for things that are not constant across a browser session (i.e. basically should only be used for feature flags and localized strings). Can we get rid of this code and rely on the getBackForwardState() call in the back_forward_button.ts constructor?
Done
This comment says back button only, but the API takes a control type. Perhaps we should change the API to just be `BackButtonHovered()`?
Done
Can we move this into back_forward_button.css? e.g. hardcode the icon as the forward one but then add some conditional CSS like:
```
blah[direction=back] {
transform: scaleX(-1);
}
```
- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
--cr-icon-button-size: 34px;Youseff BourouphelI guess your buttons would have the same touch mode issue, where the icon would be larger in touch mode. I'm trying to fix it for split tabs button. You can probably fix it after my WIP CL (https://crrev.com/c/7533005) is proved working. Just a heads up.
Thanks! Will make the fix when I see your CL is working.
yeah I think a separate CL to fix it is fine, or even preferred. Just a heads up that this needs to be fixed.
Related details
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 20 comments![]( "Open in Gerrit")
why are the changes in this file necessary?
${this.backForwardButtonEnabled_ ? html`Youseff Bourouphelditto
```suggestion
${this.backForwardButtonEnabled_ ? html`
```
Done
This doesn't look done.
* #scheme=relative
* #import=//resources/cr_elements/cr_icons_lit.css.js
* #import=//resources/cr_elements/cr_hidden_style_lit.css.js
* #include=cr-icons-lit cr-hidden-style-litcan we make this the same as split_tabs_button.css header? e.g. do we need cr_icons_lit.css?
virtual views::View* AsView() = 0;Another alternative is to remove this method and make test cast to views::View.
```suggestion
virtual views::View* AsViewForTesting() = 0;
```
auto toolbar_webview = std::make_unique<WebUIToolbarWebView>(
browser_, browser_->command_controller());
toolbar_webview_ = AddChildView(std::move(toolbar_webview));are these changes necessary?
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {Given that kWebUIBackForwardButton isn't enabled by default, do we need this code? I suspect we may want to change this test to work with the WebUI back/forward buttons rather than just disable it, but that can be done later.
// Menu model will be initialized when showing the menu to ensure it's up towhat do you mean by "up to date"? I don't see any changes to the menu model or runner below. If there aren't any changes, can we move the construction of menu_model_ and menu_runner_ into this constructor and make them actual class members rather than unique_ptrs?
browser_controls_api::mojom::ButtonState back_state;
back_state.enabled = back_control_.GetEnabled();
back_state.visible = back_control_.GetVisible();
browser_controls_api::mojom::ButtonState forward_state;
forward_state.enabled = forward_control_.GetEnabled();
forward_state.visible = forward_control_.GetVisible();can we make this use GetBackForwardButtonStates()?
UpdateBackForwardState();should this instead be PreferredSizeChanged()?
class WebUIToolbarWebViewBackForwardBrowserTest : public InProcessBrowserTest {rather than create a new class, can we reuse WebUIToolbarWebViewPixelBrowserTest?
content::ScopedAccessibilityModeOverride mode_override(ui::kAXModeComplete);
ui::TrackedElement* element = nullptr;
ASSERT_TRUE(base::test::RunUntil([&]() {
element = BrowserElements::From(browser())->GetElement(
kWebUIToolbarElementIdentifier);
return element != nullptr;
}));
ASSERT_TRUE(element);
views::TrackedElementViews* webui_toolbar_view_element =
element->AsA<views::TrackedElementViews>();
ASSERT_TRUE(webui_toolbar_view_element);
WebUIToolbarWebView* webui_toolbar_view =
views::AsViewClass<WebUIToolbarWebView>(
webui_toolbar_view_element->view());
ASSERT_TRUE(webui_toolbar_view);
views::WebView* web_view = views::AsViewClass<views::WebView>(
webui_toolbar_view->children()[0].get());
ASSERT_TRUE(web_view);can we replace this with WebUIToolbarWebViewPixelBrowserTest::SetUPWebUI()?
#include "chrome/browser/ui/views/toolbar/back_forward_control.h"why do we need this?
void SetBackForwardButtonState(```suggestion
void OnBackForwardButtonStateChanged(
```
void SetBackButtonLeadingMargin(int margin);```suggestion
void OnBackButtonLeadingMarginChanged(int margin);
```
void SetBackForwardButtonState(```suggestion
void OnBackForwardButtonStateChanged(
```
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton));can we use featuers::IsWebUIBackForward... like the above line does?
BrowserWindowInterface* browser_interface =is there a need for this change or can we revert it?
Related details
- Qingxin Wu
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 7 comments![]( "Open in Gerrit")
why is this moved down?
mistake
${this.backForwardButtonEnabled_ ? html`Youseff Bourouphelditto
```suggestion
${this.backForwardButtonEnabled_ ? html`
```
Paul JensenDone
This doesn't look done.
sorry not sure how I missed that, should be good now.
if (base::FeatureList::IsEnabled(features::kWebUIBackForwardButton)) {Given that kWebUIBackForwardButton isn't enabled by default, do we need this code? I suspect we may want to change this test to work with the WebUI back/forward buttons rather than just disable it, but that can be done later.
Acknowledged. Added a todo instead.
is there a reason to add this new line?
formatting mistake.
is there a reason this was deleted?
formatting accident.
base::FeatureList::IsEnabled(features::kWebUIBackForwardButton));can we use featuers::IsWebUIBackForward... like the above line does?
Done
is there a need for this change or can we revert it?
- Paul Jensen
- Qingxin Wu
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 10 comments![]( "Open in Gerrit")
* #scheme=relative
* #import=//resources/cr_elements/cr_icons_lit.css.js
* #import=//resources/cr_elements/cr_hidden_style_lit.css.js
* #include=cr-icons-lit cr-hidden-style-litYouseff Bourouphelcan we make this the same as split_tabs_button.css header? e.g. do we need cr_icons_lit.css?
Done, refactored to closer match split_tabs_button.css.
Another alternative is to remove this method and make test cast to views::View.
```suggestion
virtual views::View* AsViewForTesting() = 0;
```
Done
auto toolbar_webview = std::make_unique<WebUIToolbarWebView>(
browser_, browser_->command_controller());
toolbar_webview_ = AddChildView(std::move(toolbar_webview));Youseff Bourouphelare these changes necessary?
Done
browser_controls_api::mojom::ButtonState back_state;
back_state.enabled = back_control_.GetEnabled();
back_state.visible = back_control_.GetVisible();
browser_controls_api::mojom::ButtonState forward_state;
forward_state.enabled = forward_control_.GetEnabled();
forward_state.visible = forward_control_.GetVisible();Youseff Bourouphelcan we make this use GetBackForwardButtonStates()?
Done
class WebUIToolbarWebViewBackForwardBrowserTest : public InProcessBrowserTest {rather than create a new class, can we reuse WebUIToolbarWebViewPixelBrowserTest?
Done, restructured some helpers to work for any button, to reduce code needed for this and future buttons.
content::ScopedAccessibilityModeOverride mode_override(ui::kAXModeComplete);
ui::TrackedElement* element = nullptr;
ASSERT_TRUE(base::test::RunUntil([&]() {
element = BrowserElements::From(browser())->GetElement(
kWebUIToolbarElementIdentifier);
return element != nullptr;
}));
ASSERT_TRUE(element);
views::TrackedElementViews* webui_toolbar_view_element =
element->AsA<views::TrackedElementViews>();
ASSERT_TRUE(webui_toolbar_view_element);
WebUIToolbarWebView* webui_toolbar_view =
views::AsViewClass<WebUIToolbarWebView>(
webui_toolbar_view_element->view());
ASSERT_TRUE(webui_toolbar_view);
views::WebView* web_view = views::AsViewClass<views::WebView>(
webui_toolbar_view->children()[0].get());
ASSERT_TRUE(web_view);Youseff Bourouphelcan we replace this with WebUIToolbarWebViewPixelBrowserTest::SetUPWebUI()?
Done
void SetBackForwardButtonState(Youseff Bourouphel```suggestion
void OnBackForwardButtonStateChanged(
```
Done
void SetBackButtonLeadingMargin(int margin);Youseff Bourouphel```suggestion
void OnBackButtonLeadingMarginChanged(int margin);
```
Done
void SetBackForwardButtonState(Youseff Bourouphel```suggestion
void OnBackForwardButtonStateChanged(
```
Done
Related details
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 4 comments![]( "Open in Gerrit")
Youseff Bourouphelwhy are the changes in this file necessary?
Clean up to a redundant layer , which allows us not to need this.
// Menu model will be initialized when showing the menu to ensure it's up towhat do you mean by "up to date"? I don't see any changes to the menu model or runner below. If there aren't any changes, can we move the construction of menu_model_ and menu_runner_ into this constructor and make them actual class members rather than unique_ptrs?
I made `menu_model_` an actual class member to match the control's lifecycle. We still need to keep `menu_runner_` as a std::unique_ptr and recreate it in HandleContextMenu, though. views::MenuRunner is single-use, so creating a fresh instance ensures we don't carry over anything stale.
This resembles the native views version.
(https://crsrc.org/c/chrome/browser/ui/views/toolbar/toolbar_controller.cc;drc=04e6c0c420fdbc48f9ec2cf41a2ad739a949cca9;l=873)
should this instead be PreferredSizeChanged()?
Done
#include "chrome/browser/ui/views/toolbar/back_forward_control.h"Youseff Bourouphelwhy do we need this?
Removed.
Related details
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 4 comments![]( "Open in Gerrit")
This looks to be in good shape. Once you get the tests passing, fix the merge conflicts, and address my three small comments, and Qingxin also thinks it's in good shape, I think it's ready to break up into smaller CLs and start landing.
:host {```suggestion
/* TODO(crbug.com/470038385): Pass values from C++ instead of hard code when
* possible, and fix TouchUI mode */
/* TODO(crbug.com/479489814): Dedup this with split_tabs_button.css. */
:host {
```
(backState: ButtonState, forwardState: ButtonState) => {
this.updateState_(backState, forwardState);
});```suggestion
this.updateState_.bind(this));
```
should we first check that both buttons are disabled but visible?
Related details
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 4 comments![]( "Open in Gerrit")
return 'back-forward-button';nit: the other two buttons were named "xxx-button-app". Not a big deal, but would be easier to read the code if we keep the naming consistent.
private backForwardStatusChangedListenerId_: number|null = null;do we need the ids separately? Can we just put them in an array, like what we did for split_tabs_button? https://crsrc.org/c/chrome/browser/resources/webui_toolbar/split_tabs_button.ts;drc=0dbc3db913d6e3fbbb1590fdb3847e6a3307f351;l=47
It's a little easier and more extensible.
MenuSourceType.kMouse);Since ToolbarButton also inherits ToolbarButotn [0] and does not define its own handling of context menu, it goes through ToobarButton's implementation which differentiates event sources (e.g., mouse, touch, keyboard). We may also need to handle the event source type (such as touch event), as I'm doing in https://crrev.com/c/7556010 for split tabs button. We can do it in a separate CL after my CL lands. But let me know if I misunderstood how the Views back forward button handled context menu event's event source.
if (e.button !== 0) { // Only left click for long pressnit: reload_button.ts defined constants like BUTTON_LEFT. Probably good to move that to the toolbar_button.ts, and let both files use those constants.
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
aria-label="${this.ariaLabel_}"
title="${this.ariaLabel_}"are their values both this.ariaLabel_? Should title be backButtonTooltip?
https://crsrc.org/c/chrome/browser/ui/views/toolbar/back_forward_button.cc;drc=57b039617d6f75845e77204b61838efd868be036;l=44
You passed backButtonTooltip from C++ side, but seems you didn't use it in the front end code
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 8 comments![]( "Open in Gerrit")
```suggestion
/* TODO(crbug.com/470038385): Pass values from C++ instead of hard code when
* possible, and fix TouchUI mode */
/* TODO(crbug.com/479489814): Dedup this with split_tabs_button.css. */
:host {
```
Done
aria-label="${this.ariaLabel_}"
title="${this.ariaLabel_}"are their values both this.ariaLabel_? Should title be backButtonTooltip?
https://crsrc.org/c/chrome/browser/ui/views/toolbar/back_forward_button.cc;drc=57b039617d6f75845e77204b61838efd868be036;l=44
You passed backButtonTooltip from C++ side, but seems you didn't use it in the front end code
Done
nit: the other two buttons were named "xxx-button-app". Not a big deal, but would be easier to read the code if we keep the naming consistent.
Done
private backForwardStatusChangedListenerId_: number|null = null;do we need the ids separately? Can we just put them in an array, like what we did for split_tabs_button? https://crsrc.org/c/chrome/browser/resources/webui_toolbar/split_tabs_button.ts;drc=0dbc3db913d6e3fbbb1590fdb3847e6a3307f351;l=47
It's a little easier and more extensible.
Done
(backState: ButtonState, forwardState: ButtonState) => {
this.updateState_(backState, forwardState);
});Youseff Bourouphel```suggestion
this.updateState_.bind(this));
```
Done
Since ToolbarButton also inherits ToolbarButotn [0] and does not define its own handling of context menu, it goes through ToobarButton's implementation which differentiates event sources (e.g., mouse, touch, keyboard). We may also need to handle the event source type (such as touch event), as I'm doing in https://crrev.com/c/7556010 for split tabs button. We can do it in a separate CL after my CL lands. But let me know if I misunderstood how the Views back forward button handled context menu event's event source.
Done
nit: reload_button.ts defined constants like BUTTON_LEFT. Probably good to move that to the toolbar_button.ts, and let both files use those constants.
Done
should we first check that both buttons are disabled but visible?
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 5 comments![]( "Open in Gerrit")
bool IsWebUIBackForwardButtonEnabled();these changes were already submitted in the feature flag CL, right?
using ::testing::DoAll;what are this and the setargpointee used for?
MOCK_METHOD(void,
BackNavigationLikely,is this the reason we created MockWebContents instead of just using content::WebContentsTester::CreateTestWebContents? Is it possible to add this to CreateTestWebContents if we have to?
MockWebContents& mock_web_contents() {
return *static_cast<MockWebContents*>(web_contents_.get());
}what's this used for? I don't see it being used
// Tests that calling Forward() with NEW_BACKGROUND_TAB executes the IDC_FORWARDkMiddleMouseButton? Same to comments of other tests as well.
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 5 comments![]( "Open in Gerrit")
these changes were already submitted in the feature flag CL, right?
missed that from the rebase sorry!
what are this and the setargpointee used for?
outdated should have been removed before sorry.
is this the reason we created MockWebContents instead of just using content::WebContentsTester::CreateTestWebContents? Is it possible to add this to CreateTestWebContents if we have to?
Done
MockWebContents& mock_web_contents() {
return *static_cast<MockWebContents*>(web_contents_.get());
}what's this used for? I don't see it being used
Done
// Tests that calling Forward() with NEW_BACKGROUND_TAB executes the IDC_FORWARDkMiddleMouseButton? Same to comments of other tests as well.
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 5 comments![]( "Open in Gerrit")
generally LGTM with the last few comments.
std::string GetButtonAppJS(const std::string& selector) {mind separating the changes of the following three functions to a separate CL? I have a CL that also needs the exact change, so a separate CL would be helpful.
void EnableForwardButton(Browser* browser, views::WebView* web_view) {nit: let's rename it to PinForwardButton. I'm also renaming EnableSplitTabsButton.
remove the extra empty line.
And did you try running the test on mac locally? Did it fail?
// Tests that calling Forward() with CURRENT_TAB executes the IDC_FORWARDis current_tab a default parameter for forward? Forward seems takes an empty {} here.
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 4 comments![]( "Open in Gerrit")
mind separating the changes of the following three functions to a separate CL? I have a CL that also needs the exact change, so a separate CL would be helpful.
This CL is huge, I am working on smaller CLs and so this would probably get broken up, (if your CL doesn't beat me to it).
void EnableForwardButton(Browser* browser, views::WebView* web_view) {nit: let's rename it to PinForwardButton. I'm also renaming EnableSplitTabsButton.
Done
remove the extra empty line.
And did you try running the test on mac locally? Did it fail?
Removed the line and I did not test local directly, but it fails the trybots mac tests (https://ci.chromium.org/ui/p/chromium/builders/try/mac-rel/2522594/overview).
I added tests for Mac specifically since it has different behavior. Let me know if you think that is not suffice.
// Tests that calling Forward() with CURRENT_TAB executes the IDC_FORWARDis current_tab a default parameter for forward? Forward seems takes an empty {} here.
Yes, CURRENT_TAB is the default disposition for the Forward method when an empty set of ClickDispositionFlags is provided.
```
void BrowserControlsService::Forward(
const std::vector<browser_controls_api::mojom::ClickDispositionFlag>&
flags) {
command_updater_->ExecuteCommandWithDisposition(
IDC_FORWARD, ui::DispositionFromEventFlags(ToUIEventFlags(flags)));
}
```
ui::DispositionFromEventFlags by default is current_tab. As seen here: https://crsrc.org/c/ui/base/window_open_disposition_utils.h;l=31
I changed the comment to better reflect this.
Related details
- Qingxin Wu
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 2 comments![]( "Open in Gerrit")
std::string GetButtonAppJS(const std::string& selector) {Youseff Bourouphelmind separating the changes of the following three functions to a separate CL? I have a CL that also needs the exact change, so a separate CL would be helpful.
This CL is huge, I am working on smaller CLs and so this would probably get broken up, (if your CL doesn't beat me to it).
probably good to have a small CL to just refactor these functions. I'll wait for you doing that, since you already made the change.
Youseff Bourouphelremove the extra empty line.
And did you try running the test on mac locally? Did it fail?
Removed the line and I did not test local directly, but it fails the trybots mac tests (https://ci.chromium.org/ui/p/chromium/builders/try/mac-rel/2522594/overview).
I added tests for Mac specifically since it has different behavior. Let me know if you think that is not suffice.
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
std::string GetButtonAppJS(const std::string& selector) {Youseff Bourouphelmind separating the changes of the following three functions to a separate CL? I have a CL that also needs the exact change, so a separate CL would be helpful.
Qingxin WuThis CL is huge, I am working on smaller CLs and so this would probably get broken up, (if your CL doesn't beat me to it).
probably good to have a small CL to just refactor these functions. I'll wait for you doing that, since you already made the change.
unless you want me to do it, either is fine for me.
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
GetBackForwardState() => (ButtonState back_state, ButtonState forward_state);Paul made a major redesign for this inital state solution in https://chromium-review.googlesource.com/c/chromium/src/+/7552473. There are no Getxxx mojo APIs anymore. Sorry to say that, but this should also be updated to follow the new design. We can probably add a todo and change this in a separate CL if this method is not broken by that and if you're really tired of making changes in this CL :)
Related details
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 7 comments![]( "Open in Gerrit")
callbackRouter: BrowserControlsObserverCallbackRouter;why adding this?
private callbackRouter: BrowserControlsObserverCallbackRouter;why removing this, probably rebase mistake?
virtual BackForwardButton* GetBackButton() = 0;why do we need to change this?
BackForwardButton* forward_button() const { return forward_; }can these stay as ToolbarButton, or is there a reason we have to change these?
} else {
back_ = nullptr;
forward_ = nullptr;
}they're nullptr by default.
back_button_height) +just inline `GetBackForwardButtonSize().height()`?
back_->GetMinimumSize().height()) +this is MinimumSize, not PreferredSize which GetBackForwardButtonSize returns
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 8 comments![]( "Open in Gerrit")
callbackRouter: BrowserControlsObserverCallbackRouter;Youseff Bourouphelwhy adding this?
I must've messed up the rebase as this was part of whats below
private callbackRouter: BrowserControlsObserverCallbackRouter;why removing this, probably rebase mistake?
Done
virtual BackForwardButton* GetBackButton() = 0;why do we need to change this?
Done
BackForwardButton* forward_button() const { return forward_; }can these stay as ToolbarButton, or is there a reason we have to change these?
Done
} else {
back_ = nullptr;
forward_ = nullptr;
}they're nullptr by default.
Acknowledged
back_button_height) +Youseff Bouroupheljust inline `GetBackForwardButtonSize().height()`?
Done
back_->GetMinimumSize().height()) +this is MinimumSize, not PreferredSize which GetBackForwardButtonSize returns
Made GetBackForwardButtonSize have a parameter for min size.
GetBackForwardState() => (ButtonState back_state, ButtonState forward_state);Paul made a major redesign for this inital state solution in https://chromium-review.googlesource.com/c/chromium/src/+/7552473. There are no Getxxx mojo APIs anymore. Sorry to say that, but this should also be updated to follow the new design. We can probably add a todo and change this in a separate CL if this method is not broken by that and if you're really tired of making changes in this CL :)
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |