DevTools: Show HTTP status code for non-2xx prefetch failures [devtools/devtools-frontend : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@wo...@chromium.org thanks in advance for your time and guidance, please let me know if you want me to address anything!
danke!
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 4 comments![]( "Open in Gerrit")
Thanks for taking a stab at this! I like how the end result looks, added some suggestions.
const statusCodes = new Map<string, number|null>();
const networkLog = Logs.NetworkLog.NetworkLog.instance();
for (const row of rows) {
const attempt = row.pipeline.getOriginallyTriggered();
if (attempt.action === Protocol.Preload.SpeculationAction.Prefetch && 'requestId' in attempt) {
const requestId = (attempt as {requestId: Protocol.Network.RequestId}).requestId;
const requests = networkLog.requestsForId(requestId);
if (requests.length > 0) {
statusCodes.set(requestId, requests[0].statusCode);
}
}
}Maybe the `requestId` can be added to `rows` directly, such that the `statusCodes` map is not needed. Also, it might be better to move the logic outside of this view-function, which should mostly only care about rendering things.
/**
* Returns the failure reason for Non2XX prefetch errors with the status code.
* For other cases, returns null.
*/
export function prefetchFailureReasonWithStatusCode(
attempt: SDK.PreloadingModel.PrefetchAttempt, statusCode: number): string|null {
if (attempt.prefetchStatus === Protocol.Preload.PrefetchStatus.PrefetchFailedNon2XX) {
return i18nString(UIStrings.PrefetchFailedNon2XXWithStatusCode, {PH1: String(statusCode)});
}
return null;
}How about handling this in `prefetchFailureReason` without defining a new helper? `prefetchFailureReason` could take an optional `statusCode` parameter and return the appropriate string.
const detail = prefetchFailureReason(prefetchAttempt) ?? i18n.i18n.lockedString('Internal error');see above. Most of the logic can probably removed if we handle this in `prefetchFailureReason` and pass it the optional `statusCode`.
Related details
- Helmut Januschka
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)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
const statusCodes = new Map<string, number|null>();
const networkLog = Logs.NetworkLog.NetworkLog.instance();
for (const row of rows) {
const attempt = row.pipeline.getOriginallyTriggered();
if (attempt.action === Protocol.Preload.SpeculationAction.Prefetch && 'requestId' in attempt) {
const requestId = (attempt as {requestId: Protocol.Network.RequestId}).requestId;
const requests = networkLog.requestsForId(requestId);
if (requests.length > 0) {
statusCodes.set(requestId, requests[0].statusCode);
}
}
}Maybe the `requestId` can be added to `rows` directly, such that the `statusCodes` map is not needed. Also, it might be better to move the logic outside of this view-function, which should mostly only care about rendering things.
Done
* Returns the failure reason for Non2XX prefetch errors with the status code.
* For other cases, returns null.
*/
export function prefetchFailureReasonWithStatusCode(
attempt: SDK.PreloadingModel.PrefetchAttempt, statusCode: number): string|null {
if (attempt.prefetchStatus === Protocol.Preload.PrefetchStatus.PrefetchFailedNon2XX) {
return i18nString(UIStrings.PrefetchFailedNon2XXWithStatusCode, {PH1: String(statusCode)});
}
return null;
}How about handling this in `prefetchFailureReason` without defining a new helper? `prefetchFailureReason` could take an optional `statusCode` parameter and return the appropriate string.
Done
const detail = prefetchFailureReason(prefetchAttempt) ?? i18n.i18n.lockedString('Internal error');see above. Most of the logic can probably removed if we handle this in `prefetchFailureReason` and pass it the optional `statusCode`.
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 6 comments![]( "Open in Gerrit")
const originalAttempt = pipeline.getOriginallyTriggered();I'm not really familiar with preload pipelines. Why do we need to go from `attempt` to `pipeline` to `originalAttempt`?
const requests = networkLog.requestsForId(requestId);Would `originalAttempt.requestId` also work?
let statusCode: number|null = null;Since we want this to be an optional (i.e. number|undefined) parameter elsewhere, why not set it to undefined here? This allows us to avoid `statusCode?: number|null` in favor of `statusCode?: number`.
Or does have `null` a special meaning in this context?
if ('requestId' in attempt) {I think this can be removed, the type is already narrowed by the the `if` above.
statusCode?: number|null;See above. Any reason why this shouldn't be simplified to `statusCode?: number`?
const prefetchAttempt = attempt as SDK.PreloadingModel.PrefetchAttempt;Not needed, type is already narrowed.
Related details
- Helmut Januschka
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)
Helmut Januschka added 7 comments![]( "Open in Gerrit")
const originalAttempt = pipeline.getOriginallyTriggered();I'm not really familiar with preload pipelines. Why do we need to go from `attempt` to `pipeline` to `originalAttempt`?
we can use the representative attempt directly here, removed the originalAttempt indirection.
const requests = networkLog.requestsForId(requestId);Helmut JanuschkaWould `originalAttempt.requestId` also work?
Done
Since we want this to be an optional (i.e. number|undefined) parameter elsewhere, why not set it to undefined here? This allows us to avoid `statusCode?: number|null` in favor of `statusCode?: number`.
Or does have `null` a special meaning in this context?
Switched to number|undefined and defaulted to undefined when missing.
I think this can be removed, the type is already narrowed by the the `if` above.
Done
See above. Any reason why this shouldn't be simplified to `statusCode?: number`?
Done
const prefetchAttempt = attempt as SDK.PreloadingModel.PrefetchAttempt;Not needed, type is already narrowed.
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 5 comments![]( "Open in Gerrit")
Let's be consistent and ensure everywhere that `statusCode` cannot be `null`.
${hasWarning ? i18nString(UIStrings.prefetchFallbackReady) : composedStatus(attempt, row.statusCode ?? null)}remove?
{prefetchStatus}: SDK.PreloadingModel.PrefetchAttempt, statusCode?: number|null): string|null {remove?
if (statusCode !== undefined && statusCode !== null) {remove?
export function composedStatus(attempt: SDK.PreloadingModel.PreloadingAttempt, statusCode: number|null): string {```suggestion
export function composedStatus(attempt: SDK.PreloadingModel.PreloadingAttempt, statusCode?: number): string {
```
Related details
- Helmut Januschka
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)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
Let's be consistent and ensure everywhere that `statusCode` cannot be `null`.
Done
${hasWarning ? i18nString(UIStrings.prefetchFallbackReady) : composedStatus(attempt, row.statusCode ?? null)}Helmut Januschkaremove?
Done
{prefetchStatus}: SDK.PreloadingModel.PrefetchAttempt, statusCode?: number|null): string|null {Helmut Januschkaremove?
Done
if (statusCode !== undefined && statusCode !== null) {Helmut Januschkaremove?
Done
export function composedStatus(attempt: SDK.PreloadingModel.PreloadingAttempt, statusCode: number|null): string {```suggestion
export function composedStatus(attempt: SDK.PreloadingModel.PreloadingAttempt, statusCode?: number): string {
```
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Commit-Queue | +1 |
1 comment
Thanks, I think this looks great now! Sorry for coming up with another request, but could you maybe look into adding unit tests? There are existing unit tests for the changed files, so hopefully those can be used as guidance.
Related details
- Helmut Januschka
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)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Thanks, I think this looks great now! Sorry for coming up with another request, but could you maybe look into adding unit tests? There are existing unit tests for the changed files, so hopefully those can be used as guidance.
No worries at all, thanks for the feedback! its worth doing this properly. added tests
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Code-Review | +1 |
1 comment
Awesome, thanks for adding tests! LGTM
Related details
- Helmut Januschka
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 1 comment![]( "Open in Gerrit")
- Danil Somsikov
- Helmut Januschka
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 2 comments![]( "Open in Gerrit")
statusCode = requests[0].statusCode;If a prefetched request is redirected, `requestsForId` will return all requests in the redirect chain in chronological order. To show the final status code, you should use the last request from the array, not the first.
Consider changing this to:
`statusCode = requests[requests.length - 1].statusCode;`
statusCode = requests[0].statusCode;To correctly handle redirects, you should use the last request from the array to get the final status code. `requestsForId` returns requests in chronological order.
Consider changing this to:
`statusCode = requests[requests.length - 1].statusCode;`
Related details
- Helmut Januschka
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)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
If a prefetched request is redirected, `requestsForId` will return all requests in the redirect chain in chronological order. To show the final status code, you should use the last request from the array, not the first.
Consider changing this to:
`statusCode = requests[requests.length - 1].statusCode;`
Done
To correctly handle redirects, you should use the last request from the array to get the final status code. `requestsForId` returns requests in chronological order.
Consider changing this to:
`statusCode = requests[requests.length - 1].statusCode;`
- Danil Somsikov
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted Code-Review+1![]( "Open in Gerrit")
- Danil Somsikov
- Helmut Januschka
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Code-Review | +1 |
1 comment
}This logic to retrieve the status code for a prefetch attempt is also used in `PreloadingDetailsReportView.ts`. Consider extracting it into a shared helper function to avoid duplication.
Related details
- Helmut Januschka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |