Add HTTP status code to prerenderStatusUpdated CDP event [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka abandoned this change.
Helmut Januschka abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
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")
hello reviewers - thanks in advance for your help, this is the backend foundation, to be able to show http status in devtools frontend.
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7522785
Related details
- Danil Somsikov
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
- Danil Somsikov
- Helmut Januschka
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
memo: I feel that it's too complicated for adding data for one variant of failure, which can be known in the different way.
1. I feel that users can get knowing the status code by seeing the network panel, which can be navigated with a link in the preload panel.
2. I feel that this can be implemented in pure frontend, as loader ID is associated.
Related details
- Danil Somsikov
- Helmut Januschka
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
memo: I feel that it's too complicated for adding data for one variant of failure, which can be known in the different way.
1. I feel that users can get knowing the status code by seeing the network panel, which can be navigated with a link in the preload panel.
2. I feel that this can be implemented in pure frontend, as loader ID is associated.
(unresolved)
Related details
- Danil Somsikov
- Helmut Januschka
- Hiroki Nakagawa
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Ken Okadamemo: I feel that it's too complicated for adding data for one variant of failure, which can be known in the different way.
1. I feel that users can get knowing the status code by seeing the network panel, which can be navigated with a link in the preload panel.
2. I feel that this can be implemented in pure frontend, as loader ID is associated.
(unresolved)
Thanks — that makes sense, and we did consider deriving this in the frontend.
Re (1): while the status code is usually visible in the Network panel, the intent here is to make the Preload/Prerender status event self-contained so the Preload panel (and any tooling consuming the Preload domain) can show the actual failure reason without requiring a cross-panel jump and manual correlation.
Re (2): in principle we could correlate via `loaderId` in the frontend, but that would require DevTools to keep additional state and implement correlation logic between Preload events and Network requests. Since the browser already knows the HTTP status at the point we emit `NavigationBadHttpStatus`, exposing it directly on the CDP event keeps the semantics in one place and follows the existing pattern of putting “what happened” data into the protocol rather than rebuilding it in the UI.
Also, DevTools isn’t the only CDP consumer; other CDP clients can subscribe to `Preload.prerenderStatusUpdated` as well, so having the status code in the protocol is generally useful beyond the DevTools frontend.
if you see any optimizations/re-organizations/changes in the code it self, i am happy to address whatever makes the code fit!
Related details
- Danil Somsikov
- Hiroki Nakagawa
- Ken Okada
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
- Danil Somsikov
- Ken Okada
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
pong!
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
ping
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
Let me defer my review for content/browser/preloading/prerender until discussion with other DevTools experts settle down.
Feel free to set the attention bit when this needs input from me.
Related details
- Danil Somsikov
- Helmut Januschka
- Ken Okada
Danil Somsikov (Gerrit)
Danil Somsikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Code-Review | -1 |
1 comment
Ken Okadamemo: I feel that it's too complicated for adding data for one variant of failure, which can be known in the different way.
1. I feel that users can get knowing the status code by seeing the network panel, which can be navigated with a link in the preload panel.
2. I feel that this can be implemented in pure frontend, as loader ID is associated.
Helmut Januschka(unresolved)
Thanks — that makes sense, and we did consider deriving this in the frontend.
Re (1): while the status code is usually visible in the Network panel, the intent here is to make the Preload/Prerender status event self-contained so the Preload panel (and any tooling consuming the Preload domain) can show the actual failure reason without requiring a cross-panel jump and manual correlation.
Re (2): in principle we could correlate via `loaderId` in the frontend, but that would require DevTools to keep additional state and implement correlation logic between Preload events and Network requests. Since the browser already knows the HTTP status at the point we emit `NavigationBadHttpStatus`, exposing it directly on the CDP event keeps the semantics in one place and follows the existing pattern of putting “what happened” data into the protocol rather than rebuilding it in the UI.
Also, DevTools isn’t the only CDP consumer; other CDP clients can subscribe to `Preload.prerenderStatusUpdated` as well, so having the status code in the protocol is generally useful beyond the DevTools frontend.
if you see any optimizations/re-organizations/changes in the code it self, i am happy to address whatever makes the code fit!
+1 to implementing this on the client. Given that both end user feature and CDP is a mere convenience, let's aim for a minimal footprint. Find to add this as frontend feature provided a short one-pager explaining the benefits.
Related details
- Helmut Januschka
- Ken Okada
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. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
Ken Okadamemo: I feel that it's too complicated for adding data for one variant of failure, which can be known in the different way.
1. I feel that users can get knowing the status code by seeing the network panel, which can be navigated with a link in the preload panel.
2. I feel that this can be implemented in pure frontend, as loader ID is associated.
Helmut Januschka(unresolved)
Danil SomsikovThanks — that makes sense, and we did consider deriving this in the frontend.
Re (1): while the status code is usually visible in the Network panel, the intent here is to make the Preload/Prerender status event self-contained so the Preload panel (and any tooling consuming the Preload domain) can show the actual failure reason without requiring a cross-panel jump and manual correlation.
Re (2): in principle we could correlate via `loaderId` in the frontend, but that would require DevTools to keep additional state and implement correlation logic between Preload events and Network requests. Since the browser already knows the HTTP status at the point we emit `NavigationBadHttpStatus`, exposing it directly on the CDP event keeps the semantics in one place and follows the existing pattern of putting “what happened” data into the protocol rather than rebuilding it in the UI.
Also, DevTools isn’t the only CDP consumer; other CDP clients can subscribe to `Preload.prerenderStatusUpdated` as well, so having the status code in the protocol is generally useful beyond the DevTools frontend.
if you see any optimizations/re-organizations/changes in the code it self, i am happy to address whatever makes the code fit!
+1 to implementing this on the client. Given that both end user feature and CDP is a mere convenience, let's aim for a minimal footprint. Find to add this as frontend feature provided a short one-pager explaining the benefits.
I'm not an owner of CDP/frontend and it might not be common sense of CDP/frontend, but here's my opinion.
There are two oppossite positions:
P-model. CDP should provide simple model to observe and query: Like bare events, databasces, GraphQL.
P-viewmodel. CDP should provide easy to show data.
P-model prefers normalized, domain-separated data.
P-viewmodel sometimes uses non-normalized, non-domain-separated data.
What I learned from CDP/frontend development is the area what I saw prefers P-model. The easiest signal is that CDP uses domain. Off course, there may be exceptions. (I might be wrong though, but Elements panel/CSS are very complecated, both C++/frontend complecated. I won't be surprized even if those domains don't follow P-model strictly.)
In this case,
Re Re 1. Network/Preload domains should be kept seprated in CDP. We don't need to force users jump panels; we can associate data in the frontend and show them together in the Application panel.
Re Re 2. Querying in the frontend is very easy. Maintaining ad-hoc code in C++ and protocol is hard. Exception needs reason and the bar is high.
DevTools isn’t the only CDP consumer; other CDP clients can subscribe to Preload.prerenderStatusUpdated as well,
That's yet another reason why we prefer P-model.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
continuing in devtools only approach: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7620165
Related details
- Danil Somsikov
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |