Widgets for insights and network requests [devtools/devtools-frontend : main]
Alina Varkki (Gerrit)
Alina Varkki added 1 comment![]( "Open in Gerrit")
Related details
- Jack Franklin
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 7 comments![]( "Open in Gerrit")
${this.#isCollapsed ? nothing : html`
<div class="content">
<slot></slot>
</div>`}could we use a `<details>` with a `<summary>` here and then we kinda get this for free?
if (token.type === 'html') {let's leave a big comment in this file explaining that these changes are for greendev prototypes
also, let's only enter this `if` if the GreenDev experiment is enabled
// @ts-expect-errorCan we fix this? And check if the event is a synthetic network event
.requestsForId(rawTraceEventId)I think this is fine for the prototype, but let's maybe leave a comment noting that this only works for fresh traces where the network log is in sync
if (this.#data.entityMapper === data.entityMapper) {
return;
}why delete this?
(Does look like this is a bug - the check should be to not render if BOTH are equal, not one or the other)
this.#isAIAssistanceWidget = isAIAssistanceContext;nit: confusing that the property is `widget` but the setter is `context`
closed: !this.#selected || (this.#isAIAssistanceWidget),nit: superfluous parens
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alina Varkki (Gerrit)
Alina Varkki added 7 comments![]( "Open in Gerrit")
${this.#isCollapsed ? nothing : html`
<div class="content">
<slot></slot>
</div>`}could we use a `<details>` with a `<summary>` here and then we kinda get this for free?
Done
if (token.type === 'html') {let's leave a big comment in this file explaining that these changes are for greendev prototypes
also, let's only enter this `if` if the GreenDev experiment is enabled
Done
// @ts-expect-errorCan we fix this? And check if the event is a synthetic network event
It's not very clear how to fix it. `requestId` doesn't exist on the ts object, but it is actually there (I guess coming from the CDP)
.requestsForId(rawTraceEventId)I think this is fine for the prototype, but let's maybe leave a comment noting that this only works for fresh traces where the network log is in sync
added a comment. We render the tooltip if the trace is not fresh
if (this.#data.entityMapper === data.entityMapper) {
return;
}why delete this?
(Does look like this is a bug - the check should be to not render if BOTH are equal, not one or the other)
Done
this.#isAIAssistanceWidget = isAIAssistanceContext;nit: confusing that the property is `widget` but the setter is `context`
Done
closed: !this.#selected || (this.#isAIAssistanceWidget),- Jack Franklin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jack Franklin
| Code-Review | +1 |
2 comments
lgtm to not block but think we can get rid of the ts-expect-error
// @ts-expect-errorAlina VarkkiCan we fix this? And check if the event is a synthetic network event
It's not very clear how to fix it. `requestId` doesn't exist on the ts object, but it is actually there (I guess coming from the CDP)
you're reading the raw trace event, so you can use the `isSyntheticNetworkEvent` function from `Trace.Types.Events` to narrow the type
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alina Varkki (Gerrit)
Alina Varkki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alina Varkki
| Auto-Submit | +1 |
| Commit-Queue | +1 |
ptal
// @ts-expect-errorAlina VarkkiCan we fix this? And check if the event is a synthetic network event
Jack FranklinIt's not very clear how to fix it. `requestId` doesn't exist on the ts object, but it is actually there (I guess coming from the CDP)
you're reading the raw trace event, so you can use the `isSyntheticNetworkEvent` function from `Trace.Types.Events` to narrow the type
- Finnur Thorarinsson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 6 comments![]( "Open in Gerrit")
Ran out of time. I have one more file to review (the markdown formatter), but this is what I have so far. The resolved comments don't need addressing in this CL -- can be a follow-up, if you'd like.
const annotationsEnabled = Annotations.AnnotationRepository.annotationsEnabled();This can be moved in buildPreamble, as well, which would be one less thing for me to worry about. :)
"./components/CollapsibleAssistanceContentWidget.css",nit: All the other ones start with lowercase...
const {html} = Lit.StaticHtml;This has effect outside GreenDev. Is that intentional/benign?
if (this.#data.networkRequest === data.networkRequest && this.#data.entityMapper === data.entityMapper) {This used to be an 'or' but is now 'and'. Can you explain this a bit -- and (if need be) make sure it does not mess things up when the GreenDev flag is off?
// Tracks if this component is rendered withing the AI assistance panel.not: s/within/withing/
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 1 comment![]( "Open in Gerrit")
Ran out of time. I have one more file to review (the markdown formatter), but this is what I have so far. The resolved comments don't need addressing in this CL -- can be a follow-up, if you'd like.
Sorry, meant PerformanceAgentMarkdownRenderer, not formatter.
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 3 comments![]( "Open in Gerrit")
All done now (my doctor was running a bit late, so I managed to finish). :)
// Flexible regex to match the tag name and a value a.nit: Is the rest of the sentence missing?
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson added 1 comment![]( "Open in Gerrit")
All done now (my doctor was running a bit late, so I managed to finish). :)
And, like I mentioned, most of these can be ignored for now. The only thing I'm concerned about is potential effects outside GreenDev.
Related details
- Alina Varkki
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alina Varkki (Gerrit)
Alina Varkki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alina Varkki
| Auto-Submit | +1 |
2 comments
This has effect outside GreenDev. Is that intentional/benign?
yes, it's not breaking anything. I think it's from the cases when you need to dynamically set tag names or attribute names based on variables
if (this.#data.networkRequest === data.networkRequest && this.#data.entityMapper === data.entityMapper) {This used to be an 'or' but is now 'and'. Can you explain this a bit -- and (if need be) make sure it does not mess things up when the GreenDev flag is off?
Jack pointed out it was a bug.
We only need to not rerender when both values stay the same
Related details
- Finnur Thorarinsson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Finnur Thorarinsson voted![]( "Open in Gerrit")
- Alina Varkki
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Widgets for insights and network requests
- M config/gni/devtools_grd_files.gni
- M front_end/models/ai_assistance/agents/PerformanceAgent.ts
- M front_end/panels/ai_assistance/AiAssistancePanel.ts
- M front_end/panels/ai_assistance/BUILD.gn
- A front_end/panels/ai_assistance/components/CollapsibleAssistanceContentWidget.ts
- M front_end/panels/ai_assistance/components/PerformanceAgentMarkdownRenderer.ts
- A front_end/panels/ai_assistance/components/collapsibleAssistanceContentWidget.css
- M front_end/panels/timeline/components/NetworkRequestTooltip.ts
- M front_end/panels/timeline/components/insights/BaseInsightComponent.ts
- M front_end/panels/timeline/components/insights/baseInsightComponent.css
Code-Review: +1 by Finnur Thorarinsson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |