Extract presenter/view pattern for ProfileLauncherView [devtools/devtools-frontend : main]
0 views
Skip to first unread message
Simon Zünd (Gerrit)
Mar 26, 2026, 1:23:45 AMMar 26
to Helmut Januschka, Wolfgang Beyer, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Helmut Januschka and Wolfgang Beyer
Simon Zünd voted and added 3 comments![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
3 comments
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Simon Zünd . resolved
lgtm % web test failures that need fixing
File front_end/panels/profiler/ProfileLauncherView.ts
Line 80, Patchset 5 (Latest):
<label><input type="radio" name="profile-type"Simon Zünd . unresolved
I can never remember if `<input>` is one of those where it's ok to leave out the closing tag.
Slight preference of putting it, also to use `<label for=..>` as a sibling instead of nested.
Line 162, Patchset 5 (Latest):
#getControlButtonDisabled(): boolean {
return !(this.#isEnabled && this.#recordButtonEnabled);
}
#getControlButtonTooltip(): string {
return this.#recordButtonEnabled ? '' : UI.UIUtils.anotherProfilerActiveLabel();
}
#getHeaderText(): string {
return this.#profileTypes.size > 1 ? i18nString(UIStrings.selectProfilingType) :Simon Zünd . unresolved
These 3 I'd just inline directly into `performUpdate` if they don't have another call-site.
Related details
Attention is currently required from:
- Helmut Januschka
- Wolfgang Beyer
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Mar 26, 2026, 8:12:00 AMMar 26
to Helmut Januschka, Simon Zünd, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Helmut Januschka
Wolfgang Beyer added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Wolfgang Beyer . unresolved
With this change, the memory panel doesn't render for me anymore:
```
Widget.ts:162 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'widgetClass')
at Re.createWidget (Widget.ts:162:51)
at s.getOrCreateWidget (Widget.ts:495:22)
at Widget.ts:328:27
```
File front_end/panels/profiler/IsolateSelector.ts
Line 69, Patchset 5 (Latest):
constructor(element?: HTMLElement) {
super(element);Wolfgang Beyer . unresolved
What caused the need for this change?
File front_end/panels/profiler/ProfileLauncherView.ts
Line 101, Patchset 5 (Latest):
.widgetConfig=${widgetConfig(IsolateSelector)}Wolfgang Beyer . unresolved
There is a newer way for this now: `${widget(IsolateSelector)}`
Related details
Attention is currently required from:
- Helmut Januschka
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Apr 3, 2026, 8:14:15 AMApr 3
to Helmut Januschka, Simon Zünd, Wolfgang Beyer, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Simon Zünd and Wolfgang Beyer
Helmut Januschka added 5 comments![]( "Open in Gerrit")
Patchset-level comments
With this change, the memory panel doesn't render for me anymore:
```
Widget.ts:162 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'widgetClass')
at Re.createWidget (Widget.ts:162:51)
at s.getOrCreateWidget (Widget.ts:495:22)
at Widget.ts:328:27
```
Helmut Januschka
gosh i missed that
File front_end/panels/profiler/IsolateSelector.ts
What caused the need for this change?
Helmut Januschka
This became necessary when `ProfileLauncherView` started creating `IsolateSelector` via `<devtools-widget>` (`widgetConfig(IsolateSelector)`). In that path the widget constructor is called with the host element, so `IsolateSelector` must accept `element?: HTMLElement` and pass it to `super(element)`; otherwise it would attach to a fresh `<div>` instead of the `devtools-widget` host. I added an inline constructor comment to make this explicit.
File front_end/panels/profiler/ProfileLauncherView.ts
I can never remember if `<input>` is one of those where it's ok to leave out the closing tag.
Slight preference of putting it, also to use `<label for=..>` as a sibling instead of nested.
Helmut Januschka
switched the radio markup to sibling elements with `id`/`for` (`<input ... />` + `<label for=...>`), instead of nesting the input inside the label.
There is a newer way for this now: `${widget(IsolateSelector)}`
Helmut Januschka
Done
#getControlButtonDisabled(): boolean {
return !(this.#isEnabled && this.#recordButtonEnabled);
}
#getControlButtonTooltip(): string {
return this.#recordButtonEnabled ? '' : UI.UIUtils.anotherProfilerActiveLabel();
}
#getHeaderText(): string {
return this.#profileTypes.size > 1 ? i18nString(UIStrings.selectProfilingType) :Simon Zünd . resolved
These 3 I'd just inline directly into `performUpdate` if they don't have another call-site.
Attention is currently required from:
- Simon Zünd
- Wolfgang Beyer
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Apr 7, 2026, 3:59:45 AMApr 7
to Helmut Januschka, Wolfgang Beyer, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Helmut Januschka and Wolfgang Beyer
Simon Zünd voted and added 1 comment![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
1 comment
File front_end/panels/profiler/ProfileLauncherView.ts
Line 106, Patchset 10 (Latest):
${widget(input.isolateSelectorFactory)}Simon Zünd . unresolved
Ah, you also need a ref to it. The canonical way to do this is a bit verbose at the moment but looks like:
```
<devtools-widget
${widget(IsolateSelector)}
${widgetRef(IsolateSelector, e => {output.isolateSelector = e;})}
></devtools-widget>
```
Related details
Attention is currently required from:
- Helmut Januschka
- Wolfgang Beyer
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Jun 7, 2026, 3:21:03 PM (7 days ago) Jun 7
to Helmut Januschka, Daevil Deighton, Simon Zünd, Wolfgang Beyer, devtools-fro...@luci-project-accounts.iam.gserviceaccount.com, devtools-rev...@chromium.org
Attention needed from Simon Zünd
Helmut Januschka voted and added 1 comment![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
1 comment
File front_end/panels/profiler/ProfileLauncherView.ts
Ah, you also need a ref to it. The canonical way to do this is a bit verbose at the moment but looks like:
```
<devtools-widget
${widget(IsolateSelector)}
${widgetRef(IsolateSelector, e => {output.isolateSelector = e;})}
></devtools-widget>
```
Attention is currently required from:
- Simon Zünd
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Jun 8, 2026, 12:29:41 AM (6 days ago) Jun 8
to Helmut Januschka, Daevil Deighton, Wolfgang Beyer, devtools-fro...@luci-project-accounts.iam.gserviceaccount.com, devtools-rev...@chromium.org
Attention needed from Helmut Januschka
Simon Zünd voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Helmut Januschka
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Jun 9, 2026, 10:27:26 AM (5 days ago) Jun 9
to Helmut Januschka, Simon Zünd, Daevil Deighton, devtools-fro...@luci-project-accounts.iam.gserviceaccount.com, devtools-rev...@chromium.org
Attention needed from Helmut Januschka
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Helmut Januschka
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Jun 13, 2026, 3:17:59 AM (yesterday) Jun 13
to Helmut Januschka, Wolfgang Beyer, Simon Zünd, Daevil Deighton, devtools-fro...@luci-project-accounts.iam.gserviceaccount.com, devtools-rev...@chromium.org
Helmut Januschka voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
devtools-frontend-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
Jun 13, 2026, 4:01:29 AM (yesterday) Jun 13
to Helmut Januschka, Wolfgang Beyer, Simon Zünd, Daevil Deighton, devtools-rev...@chromium.org
devtools-fro...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Extract presenter/view pattern for ProfileLauncherView
Migrate ProfileLauncherView from imperative DOM construction to the
declarative presenter/view pattern.
Bug: 400353541
Change-Id: Ic0fa6407ab23e46905e774d142d7836dd4415e0a
Reviewed-by: Wolfgang Beyer <wo...@chromium.org>
Reviewed-by: Simon Zünd <szu...@chromium.org>
Commit-Queue: Helmut Januschka <hel...@januschka.com>
Files:
- M front_end/panels/profiler/IsolateSelector.ts
- M front_end/panels/profiler/ProfileLauncherView.ts
- M front_end/panels/profiler/ProfilesPanel.ts
Change size: L
Delta: 3 files changed, 198 insertions(+), 130 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Wolfgang Beyer, +1 by Simon Zünd
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages