Add Plus Button (2/3): Add declarative plus-button option to TabbedLocation [devtools/devtools-frontend : main]
Guangyue Xu (Gerrit)
Guangyue Xu added 1 comment![]( "Open in Gerrit")
Hi Danil and Piotr,
Could you take a look at this CL when you have a moment? It's the second of three planned CLs for the Plus Button feature, building on CL1's `TabbedPane` slot + overflow event. This one adds the populator/installer in a new `PlusButton.ts` and a declarative `plusButton` option on `createTabbedLocation`. No user-visible change yet — CL3 will opt the panel and drawer locations in.
For context:
Prototype CL: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7792390
CL1 (landed): https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7814020
Thanks,
Guangyue Xu
Related details
- Danil Somsikov
- Piotr Paulski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 5 comments![]( "Open in Gerrit")
// Compare against the same exported constant the implementation
// uses, not a hard-coded English string.
assert.strictEqual(button.title, UI.PlusButton.DEFAULT_TITLE());This is not worth the complexity. You can test against the English string, test only run against the English locale
export function populatePlusButtonMenu(This logic for gathering, filtering, and deduplicating views feels quite imperative. To follow our Model-View-Presenter (MVP) architecture, this business logic should be moved into a Presenter class. This separates the logic of 'what to show' from the legacy UI logic.
if (view.viewId() === 'issues-pane') {You can avoid the duplication here and correctly preserve `view.isPreviewFeature()` instead of hardcoding `false` by merging these two blocks:
```typescript
const isIssuesPane = view.viewId() === 'issues-pane';
addToolEntries.push({
title: view.title(),
jslogContext: view.viewId(),
isPreviewFeature: view.isPreviewFeature(),
action: () => {
if (isIssuesPane) {
Host.userMetrics.issuesPanelOpenedFrom(Host.UserMetrics.IssueOpener.HAMBURGER_MENU);
}
showView(view);
},
});
```
Also, a quick question: is `HAMBURGER_MENU` the correct metric enum to use here, or should the Plus Button get its own enum value (like `MORE_TOOLS_MENU` or similar) to avoid mixing this data with the main DevTools three-dot menu?
context.tabbedPane.element.appendChild(button);We should avoid using imperative APIs like `appendChild` to update the DOM. Instead, we should rely on orchestrated rendering of lit-html templates. The plus button should ideally be rendered via a declarative view function.
if (options?.plusButton) {This new feature must be guarded by an experiment flag. Please ensure that the plus button is only installed if the corresponding experiment is enabled, for example: `if (options?.plusButton && Root.Runtime.experiments.isEnabled('plus-button'))`.
Related details
- Guangyue Xu
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 6 comments![]( "Open in Gerrit")
// Compare against the same exported constant the implementation
// uses, not a hard-coded English string.
assert.strictEqual(button.title, UI.PlusButton.DEFAULT_TITLE());This is not worth the complexity. You can test against the English string, test only run against the English locale
Done. The default-tooltip assertion in the `installPlusButton` describe block now compares against the literal 'More tools' (no `i18nString` round-trip), matching the convention used elsewhere.
export function populatePlusButtonMenu(This logic for gathering, filtering, and deduplicating views feels quite imperative. To follow our Model-View-Presenter (MVP) architecture, this business logic should be moved into a Presenter class. This separates the logic of 'what to show' from the legacy UI logic.
Done. Extracted `PlusButtonPresenter` with a `buildModel(): PlusButtonMenuModel` method (lines ~138-235). All gathering / filtering / dedup / sort lives there; `populatePlusButtonMenu` is now a thin "view" that takes the model and pushes it into `ContextMenu`. Presenter is exported and unit-tested directly (`describe('PlusButtonPresenter')`).
if (view.viewId() === 'issues-pane') {You can avoid the duplication here and correctly preserve `view.isPreviewFeature()` instead of hardcoding `false` by merging these two blocks:
```typescript
const isIssuesPane = view.viewId() === 'issues-pane';
addToolEntries.push({
title: view.title(),
jslogContext: view.viewId(),
isPreviewFeature: view.isPreviewFeature(),
action: () => {
if (isIssuesPane) {
Host.userMetrics.issuesPanelOpenedFrom(Host.UserMetrics.IssueOpener.HAMBURGER_MENU);
}
showView(view);
},
});
```Also, a quick question: is `HAMBURGER_MENU` the correct metric enum to use here, or should the Plus Button get its own enum value (like `MORE_TOOLS_MENU` or similar) to avoid mixing this data with the main DevTools three-dot menu?
Done — merged the two branches; the issues-pane case now reuses `view.isPreviewFeature()` and only the UMA call is conditional on `isIssuesPane`.
On the `enum:` agreed, added a new `IssueOpener.MORE_TOOLS_MENU = 6` in `UserMetrics.ts:749-758` (with a JSDoc comment pointing to the matching Chromium `enum DevToolsIssuesPanelOpenedFrom` in `tools/metrics/histograms/metadata/dev/enums.xml`). The plus-button issues entry now records `MORE_TOOLS_MENU` instead of `HAMBURGER_MENU` so the dashboard doesn't conflate the two surfaces. Filing the matching Chromium-side `enums.xml` CL as a follow-up.
if (otherLocation) {Removed the `includeOtherMainLocation` opt-in: cross-location is the design intent, not a per-caller knob. Per the design doc, the plus button replaces the "More tools" ≫ button to make the experience symmetric across the main panel and drawer — "the context menu for the '+' button in the main panel would show the tools not already available in the main panel, and the same holds for the drawer panel." The presenter still gates the loop on `location ∈ {PANEL, DRAWER_VIEW}`, so non-main tabbed locations are unaffected.
context.tabbedPane.element.appendChild(button);We should avoid using imperative APIs like `appendChild` to update the DOM. Instead, we should rely on orchestrated rendering of lit-html templates. The plus button should ideally be rendered via a declarative view function.
Done. `installPlusButton` now uses a single `tabbedPane.element`). No more `document.createElement` / `setAttribute` / `appendChild`. The `Directives.ref` capture replaces the previous `querySelector` round-trip so we still hand the caller back the `MenuButton` instance.
if (options?.plusButton) {This new feature must be guarded by an experiment flag. Please ensure that the plus button is only installed if the corresponding experiment is enabled, for example: `if (options?.plusButton && Root.Runtime.experiments.isEnabled('plus-button'))`.
Done. Registered a `plus-button` experiment in `ExperimentNames.ts` (`PLUS_BUTTON = 'plus-button'`) and `MainImpl.ts`, and added it to the `DevtoolsExperiments` UMA enum (value 112, MAX bumped to 113). The install site in `ViewManager.ts:724` now reads:
```
if (options?.plusButton && Root.Runtime.experiments.isEnabled(Root.ExperimentNames.ExperimentName.PLUS_BUTTON)) { PlusButton.installPlusButton(...);}
```
Also added a regression test in `ViewManager.test.ts` that flips the experiment and asserts the button mounts before `appendApplicableItems`. Test-only experiment registration mirrored in `RuntimeHelpers.ts`.
Related details
- Danil Somsikov
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 2 comments![]( "Open in Gerrit")
Root.Runtime.experiments.register(
Root.ExperimentNames.ExperimentName.PLUS_BUTTON, 'Show "+" button on the tab strip for adding tools');
Please don't add new Runtime.experiments
// Skip views that are already shown as a visible tab — the user can`hasTab` returns true for both visible and hidden tabs. You might want to update this comment to clarify that hidden tabs are skipped here because they are already handled in the `overflowTabs` section above.
For example:
```typescript
// Skip views that already have a tab. Hidden tabs are listed in the overflow section,
// and visible tabs are accessible directly in the tab strip.
```
Related details
- Guangyue Xu
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |