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. |