ServiceWorker: add typed version of routerRules CDP field for DevTools [chromium/src : main]
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alex Rudenko
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted Commit-Queue+0![]( "Open in Gerrit")
- Alex Rudenko
- Takashi Nakayama
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 4 comments![]( "Open in Gerrit")
std::optional<std::string> legacy_router_rules;This block is duplicated between `ServiceWorkerVersion::GetInfo()` and `ServiceWorkerContextCore::OnVersionStateChanged()`.
Can we consider adding a helper method on `ServiceWorkerVersion` (e.g., `GetRouterRulesForDevTools()`), or `ServiceWorkerRouterEvaluator()`?
// Router rules that have nested conditinos are currently unsupported. UseTypo: "conditinos" -> "conditions".
// Router rules that have nested conditinos are currently unsupported. UseTypo: "conditinos" -> "conditions".
std::vector<ServiceWorkerRouterRule> PrepareRouterRules() const;The name "PrepareRouterRules" is a bit generic and not clear what the function will do. Consider a name that matches the destination or the transformation, such as `GetTypedRouterRules()` (matching the CDP field) or `GetRouterRulesForDevTools()`.
Related details
- Alex Rudenko
- Takashi Nakayama
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 4 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
4 comments
std::vector<ServiceWorkerRouterRule> PrepareRouterRules() const;The name "PrepareRouterRules" is a bit generic and not clear what the function will do. Consider a name that matches the destination or the transformation, such as `GetTypedRouterRules()` (matching the CDP field) or `GetRouterRulesForDevTools()`.
+1
I failed to catch what would be returned from `PreapreRouterRules()`,
std::string source;Ah, is source so simple?
I thought the source would be represented with union.
# Plain text or JSON serialization of `RouterSourceDict` in the specRouterSourceEnum will be a plain text?
Related details
- Alex Rudenko
- Takashi Nakayama
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 5 comments![]( "Open in Gerrit")
Please add inspector-protocol tests covering the new type output (ideally for various possible outputs).
// Router rules that have nested conditinos are currently unsupported. Usenit: conditinos -> conditions
std::optional<std::string> router_rules,Consider passing these by `const &` (e.g., `const std::optional<std::string>&` and `const std::vector<ServiceWorkerRouterRule>&`) to avoid unnecessary copies when notifying multiple observers. The previous `router_rules` parameter was passed by `const &`.
If you update this, remember to also update the overrides in `ServiceWorkerContextWatcher` and `ServiceWorkerInternalsUI`.
number idnit: Since rule IDs are typically whole numbers, consider using `integer id` instead of `number` in the DevTools protocol.
# `routerRules` or `typedRouterRules`.do we plan to rename typedRouterRules back to routerRules?
Related details
- Takashi Nakayama
Takashi Nakayama (Gerrit)
Takashi Nakayama added 11 comments![]( "Open in Gerrit")
Please add inspector-protocol tests covering the new type output (ideally for various possible outputs).
Could we add them together with the implementation of `ServiceWorkerRouterEvaluator::CalculateRouterRulesForDevTools()` in `content/common/service_worker/service_worker_router_evaluator.cc`?
std::optional<std::string> legacy_router_rules;This block is duplicated between `ServiceWorkerVersion::GetInfo()` and `ServiceWorkerContextCore::OnVersionStateChanged()`.
Can we consider adding a helper method on `ServiceWorkerVersion` (e.g., `GetRouterRulesForDevTools()`), or `ServiceWorkerRouterEvaluator()`?
Added `ServiceWorkerVersion::CalculateRouterRulesForDevTools()`. PTAL
// Router rules that have nested conditinos are currently unsupported. UseTypo: "conditinos" -> "conditions".
Done
// Router rules that have nested conditinos are currently unsupported. UseTakashi Nakayamanit: conditinos -> conditions
Done
std::optional<std::string> router_rules,Consider passing these by `const &` (e.g., `const std::optional<std::string>&` and `const std::vector<ServiceWorkerRouterRule>&`) to avoid unnecessary copies when notifying multiple observers. The previous `router_rules` parameter was passed by `const &`.
If you update this, remember to also update the overrides in `ServiceWorkerContextWatcher` and `ServiceWorkerInternalsUI`.
Changed to use a new `ServiceWorkerVersion::RouterRulesForDevTools` struct and pass its const reference. PTAL
// Router rules that have nested conditinos are currently unsupported. UseTypo: "conditinos" -> "conditions".
Done
std::vector<ServiceWorkerRouterRule> PrepareRouterRules() const;Yoshisato YanagisawaThe name "PrepareRouterRules" is a bit generic and not clear what the function will do. Consider a name that matches the destination or the transformation, such as `GetTypedRouterRules()` (matching the CDP field) or `GetRouterRulesForDevTools()`.
+1
I failed to catch what would be returned from `PreapreRouterRules()`,
Thanks for the suggestion. Renamed to `CalculateRouterRulesForDevTools()`.
Ah, is source so simple?
I thought the source would be represented with union.
Changed to use `blink::ServiceWorkerRouterSource` according to the PDL change.
# Plain text or JSON serialization of `RouterSourceDict` in the specRouterSourceEnum will be a plain text?
Added new types to RouterSource in the spec and changed to use it. PTAL
nit: Since rule IDs are typically whole numbers, consider using `integer id` instead of `number` in the DevTools protocol.
Done
# `routerRules` or `typedRouterRules`.do we plan to rename typedRouterRules back to routerRules?
No. I'm not sure about the backward compatibility policy of DevTools, but dsv@ suggested keeping the payload of `routerRule` for the backward compatibility. DevTools will keep the support of the legacy `routerRules` field while the browser is planned to send only `typedRouterRules` afterwards (IIUC).
Related details
- Alex Rudenko
- Shunya Shishido
- Yoshisato Yanagisawa
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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
# `routerRules` or `typedRouterRules`.Takashi Nakayamado we plan to rename typedRouterRules back to routerRules?
No. I'm not sure about the backward compatibility policy of DevTools, but dsv@ suggested keeping the payload of `routerRule` for the backward compatibility. DevTools will keep the support of the legacy `routerRules` field while the browser is planned to send only `typedRouterRules` afterwards (IIUC).
This is an experimental domain. Perhaps we could just replace routerRules with the new content and address the issues during the roll? wdyt @d...@chromium.org?
Related details
- Shunya Shishido
- Takashi Nakayama
- Yoshisato Yanagisawa
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Takashi NakayamaPlease add inspector-protocol tests covering the new type output (ideally for various possible outputs).
Could we add them together with the implementation of `ServiceWorkerRouterEvaluator::CalculateRouterRulesForDevTools()` in `content/common/service_worker/service_worker_router_evaluator.cc`?
Which CL is that? is it already chained to this CL? I would prefer adding tests in the same CL to make sure everything works as expected.
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
1 comment
std::optional<std::string> legacy_router_rules;Takashi NakayamaThis block is duplicated between `ServiceWorkerVersion::GetInfo()` and `ServiceWorkerContextCore::OnVersionStateChanged()`.
Can we consider adding a helper method on `ServiceWorkerVersion` (e.g., `GetRouterRulesForDevTools()`), or `ServiceWorkerRouterEvaluator()`?
Added `ServiceWorkerVersion::CalculateRouterRulesForDevTools()`. PTAL
- Takashi Nakayama
- Yoshisato Yanagisawa
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
Takashi NakayamaPlease add inspector-protocol tests covering the new type output (ideally for various possible outputs).
Alex RudenkoCould we add them together with the implementation of `ServiceWorkerRouterEvaluator::CalculateRouterRulesForDevTools()` in `content/common/service_worker/service_worker_router_evaluator.cc`?
Which CL is that? is it already chained to this CL? I would prefer adding tests in the same CL to make sure everything works as expected.
The CL hasn't been present yet. Since I would like to delegate the task to another person, let me split the CL into the two: one for the browser's internal changes and the other for the parts that need CDP changes.
Takashi NakayamaConsider passing these by `const &` (e.g., `const std::optional<std::string>&` and `const std::vector<ServiceWorkerRouterRule>&`) to avoid unnecessary copies when notifying multiple observers. The previous `router_rules` parameter was passed by `const &`.
If you update this, remember to also update the overrides in `ServiceWorkerContextWatcher` and `ServiceWorkerInternalsUI`.
Changed to use a new `ServiceWorkerVersion::RouterRulesForDevTools` struct and pass its const reference. PTAL
- Alex Rudenko
- Yoshisato Yanagisawa
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
Takashi NakayamaPlease add inspector-protocol tests covering the new type output (ideally for various possible outputs).
Alex RudenkoCould we add them together with the implementation of `ServiceWorkerRouterEvaluator::CalculateRouterRulesForDevTools()` in `content/common/service_worker/service_worker_router_evaluator.cc`?
Takashi NakayamaWhich CL is that? is it already chained to this CL? I would prefer adding tests in the same CL to make sure everything works as expected.
The CL hasn't been present yet. Since I would like to delegate the task to another person, let me split the CL into the two: one for the browser's internal changes and the other for the parts that need CDP changes.
I mean, the upcoming CL for the type converter implementation and new inspector-protocol tests will be chained to this CL, and we will submit the upcoming CL and this CL (CDP changes) together.
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Takashi NakayamaPlease add inspector-protocol tests covering the new type output (ideally for various possible outputs).
Alex RudenkoCould we add them together with the implementation of `ServiceWorkerRouterEvaluator::CalculateRouterRulesForDevTools()` in `content/common/service_worker/service_worker_router_evaluator.cc`?
Takashi NakayamaWhich CL is that? is it already chained to this CL? I would prefer adding tests in the same CL to make sure everything works as expected.
Takashi NakayamaThe CL hasn't been present yet. Since I would like to delegate the task to another person, let me split the CL into the two: one for the browser's internal changes and the other for the parts that need CDP changes.
I mean, the upcoming CL for the type converter implementation and new inspector-protocol tests will be chained to this CL, and we will submit the upcoming CL and this CL (CDP changes) together.
I'd prefer to add required tests in this CL so that the protocol change and the tests can be reviewed together. Please ask me to review once you have a CL with tests ready.
Related details
- Takashi Nakayama
- Yoshisato Yanagisawa
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
2 comments
# Plain text or JSON serialization of `RouterSourceDict` in the specTakashi NakayamaRouterSourceEnum will be a plain text?
Added new types to RouterSource in the spec and changed to use it. PTAL
Acknowledged
Related details
- Takashi Nakayama
Sora Kobayashi (Gerrit)
Sora Kobayashi voted Commit-Queue+1![]( "Open in Gerrit")
- Keita Suzuki
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 5 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Code-Review | +1 |
5 comments
This test-only CL adds inspector-protocol tests for the new fields.This CL is not test-only. How about this?
```suggestion
This CL also adds inspector-protocol tests for the new fields.
```
Note: This is separated from the main implementation CL
(crrev.com/c/8182989) due to permission issues when uploading the test
patchset.Let me draft the message to avoid any misunderstandings.
```suggestion
Note: The non-test parts of this CL are directly taken from crrev.com/c/8182989 instead of creating chained CLs. This is due to internal permission errors and approved by the original author (tn...@chromium.org).
```
"prefix": "static-routing-api",How about `static-routing-api-devtools-improvement`?
async function waitForServiceWorkerInstalled() {nit: I'd prefer `InstalledEvent` or `Installation`.
const versionsPromise = waitForServiceWorkerInstalled();ditto
Related details
- Keita Suzuki
- Sora Kobayashi
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. |
Keita Suzuki (Gerrit)
Keita Suzuki added 1 comment![]( "Open in Gerrit")
testRunner.log(versions[0].typedRouterRules);Could we add comments on what we are actually logging here?
Same applies for the IDs.
Related details
- Sora Kobayashi
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. |
Keita Suzuki (Gerrit)
Keita Suzuki added 1 comment![]( "Open in Gerrit")
Note: The non-test parts of this CL are directly taken from crrev.com/c/8182989 instead of creating chained CLs. This is due to internal permission errors and approved by the original author (tn...@chromium.org).please add an empty line after this (and also please format).
Related details
- Sora Kobayashi
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama abandoned this change.
Takashi Nakayama abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sora Kobayashi (Gerrit)
Sora Kobayashi added 7 comments![]( "Open in Gerrit")
This test-only CL adds inspector-protocol tests for the new fields.This CL is not test-only. How about this?
```suggestion
This CL also adds inspector-protocol tests for the new fields.
```
Done
Note: The non-test parts of this CL are directly taken from crrev.com/c/8182989 instead of creating chained CLs. This is due to internal permission errors and approved by the original author (tn...@chromium.org).please add an empty line after this (and also please format).
Done
Note: This is separated from the main implementation CL
(crrev.com/c/8182989) due to permission issues when uploading the test
patchset.Let me draft the message to avoid any misunderstandings.
```suggestion
Note: The non-test parts of this CL are directly taken from crrev.com/c/8182989 instead of creating chained CLs. This is due to internal permission errors and approved by the original author (tn...@chromium.org).
Sora Kobayashi```
Done
"prefix": "static-routing-api",Sora KobayashiHow about `static-routing-api-devtools-improvement`?
Done
nit: I'd prefer `InstalledEvent` or `Installation`.
Done
const versionsPromise = waitForServiceWorkerInstalled();Sora Kobayashiditto
Done
testRunner.log(versions[0].typedRouterRules);Could we add comments on what we are actually logging here?
Same applies for the IDs.
- Keita Suzuki
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 3 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Code-Review | +1 |
3 comments
%testcode LGTM w/ nit.
Since the non-test code was written by myself, please make sure to ask other commiters for reviews.
async function waitForServiceWorkerInstalled() {Sora Kobayashinit: I'd prefer `InstalledEvent` or `Installation`.
Done
nit: let me reopen this comment because it's not done yet.
const versionsPromise = waitForServiceWorkerInstalled();Sora Kobayashiditto
Takashi NakayamaDone
ditto
Related details
- Keita Suzuki
- Sora Kobayashi
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. |
Keita Suzuki (Gerrit)
Keita Suzuki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Keita Suzuki
| Code-Review | +1 |
2 comments
LGTM from myside
testRunner.log(versions[0].typedRouterRules);Sora KobayashiCould we add comments on what we are actually logging here?
Same applies for the IDs.
Done. Added comments explaining the log output.
- Sora Kobayashi
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. |
Sora Kobayashi (Gerrit)
Sora Kobayashi added 2 comments![]( "Open in Gerrit")
async function waitForServiceWorkerInstalled() {Sora Kobayashinit: I'd prefer `InstalledEvent` or `Installation`.
Takashi NakayamaDone
nit: let me reopen this comment because it's not done yet.
Done
const versionsPromise = waitForServiceWorkerInstalled();Sora Kobayashiditto
Takashi NakayamaDone
ditto
- Keita Suzuki
- Shunya Shishido
- Takashi Nakayama
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keita Suzuki (Gerrit)
Keita Suzuki voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keita Suzuki
| Code-Review | +1 |
- Shunya Shishido
- Sora Kobayashi
- Takashi Nakayama
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Code-Review+1![]( "Open in Gerrit")
- Shunya Shishido
- Sora Kobayashi
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 3 comments![]( "Open in Gerrit")
ConvertRouterSource(const blink::ServiceWorkerRouterSource& source) {Non-blocking but can we have more tests? Especially for:
- RaceNetworkAndFetchEvent source
- Cache source with the cache name
- Cache source only
NOTREACHED();Can we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
type ServiceWorkerRouterSource extends objectRouterSource is `RouterSouceDict` OR `RouterSourceEnum`.
https://www.w3.org/TR/service-workers/#typedefdef-routersource
OTOH this CL makes `ServiceWorkerRouterSourceType` which supports "sourceDict" and optional `ServiceWorkerRouterSourceDict`.
Is this intended due to the restriction by PDL or something? If so please leave comment why we choose this interface which is not same as the spec?
Related details
- Sora Kobayashi
- Yoshisato Yanagisawa
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
NOTREACHED();Can we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
It is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
type ServiceWorkerRouterSource extends objectRouterSource is `RouterSouceDict` OR `RouterSourceEnum`.
https://www.w3.org/TR/service-workers/#typedefdef-routersourceOTOH this CL makes `ServiceWorkerRouterSourceType` which supports "sourceDict" and optional `ServiceWorkerRouterSourceDict`.
Is this intended due to the restriction by PDL or something? If so please leave comment why we choose this interface which is not same as the spec?
This is due to the restriction by PDL. @otu...@google.com Could you add comment like this?
```suggestion
# Corresponds to `RouterSource` in the spec while the representation is different as follows.
# (https://www.w3.org/TR/service-workers/#typedefdef-routersource)
# - `RouterSourceEnum`: `type` equals `cache`, `sourceDict` is null.
# - `RouterSourceDict`: `type` equals `sourceDict`, `sourceDict` has valid value.
type ServiceWorkerRouterSource extends object
```
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 4 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
4 comments
lgtm w/ comments.
NOTREACHED();Takashi NakayamaCan we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
It is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
I think it is good to be left as-is.The RaceNetworkAndCache case is just not implemented case, and it is not strange the code would eventually be executed. However, this NOTREACHED() must not happen, or the compiler is broken.
CHECK(!or_condition.has_value() && !not_condition.has_value())
<< "nested conditions are not yet supported.";It might be good to leave TODO?
Both should be used in practice, and I believe having them are unavoidable to productinize this feature.
CHECK(!version.router_rules);I failed to catch the meaning of this `CHECK`. It looks trivial upon L516, and I assume having `router_rules` can be used as a switch to go with the legacy implementation or the new implementation, but will the code after this may break if `version.router_rules` exist?
Related details
- Sora Kobayashi
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. |
Shunya Shishido (Gerrit)
Shunya Shishido added 3 comments![]( "Open in Gerrit")
NOTREACHED();Takashi NakayamaCan we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
It is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
Sounds good but then we don't need `NOTIMPLEMENTED()` I think.
CHECK(!or_condition.has_value() && !not_condition.has_value())Are we really sure that the router condition has neither `or_condition` nor `not_condition` here? If so who guarantees it? Since this causes the browser crash, we should carefully ensure that and clarify why it's safe.
condition->SetRunningStatus("running");Please fix this WARNING reported by autoreview issue finding: For consistency with the rest of this file (see line 50), consider using the protocol constants: `protocol::ServiceWorker::ServiceWorkerVersionRunningStatusEnum::Running` and `protocol::ServiceWorker::ServiceWorkerVersionRunningStatusEnum::Stopped`.
Related details
- Keita Suzuki
- Sora Kobayashi
- Takashi Nakayama
- Yoshisato Yanagisawa
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. |
Sora Kobayashi (Gerrit)
Sora Kobayashi added 1 comment![]( "Open in Gerrit")
ConvertRouterSource(const blink::ServiceWorkerRouterSource& source) {Non-blocking but can we have more tests? Especially for:
- RaceNetworkAndFetchEvent source
- Cache source with the cache name
- Cache source only
Thanks for the suggestion! I'll add these in a follow-up CL. I've filed crbug.com/548227769 to track this and added a TODO in the test file.
Related details
- Keita Suzuki
- Shunya Shishido
- Takashi Nakayama
- Yoshisato Yanagisawa
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. |
Sora Kobayashi (Gerrit)
Sora Kobayashi added 1 comment![]( "Open in Gerrit")
ConvertRouterSource(const blink::ServiceWorkerRouterSource& source) {Sora KobayashiNon-blocking but can we have more tests? Especially for:
- RaceNetworkAndFetchEvent source
- Cache source with the cache name
- Cache source only
Thanks for the suggestion! I'll add these in a follow-up CL. I've filed crbug.com/548227769 to track this and added a TODO in the test file.
Acknowledged
Sora Kobayashi (Gerrit)
Sora Kobayashi added 1 comment![]( "Open in Gerrit")
Takashi NakayamaRouterSource is `RouterSouceDict` OR `RouterSourceEnum`.
https://www.w3.org/TR/service-workers/#typedefdef-routersourceOTOH this CL makes `ServiceWorkerRouterSourceType` which supports "sourceDict" and optional `ServiceWorkerRouterSourceDict`.
Is this intended due to the restriction by PDL or something? If so please leave comment why we choose this interface which is not same as the spec?
This is due to the restriction by PDL. @otu...@google.com Could you add comment like this?
```suggestion
# Corresponds to `RouterSource` in the spec while the representation is different as follows.
# (https://www.w3.org/TR/service-workers/#typedefdef-routersource)
# - `RouterSourceEnum`: `type` equals `cache`, `sourceDict` is null.
# - `RouterSourceDict`: `type` equals `sourceDict`, `sourceDict` has valid value.
type ServiceWorkerRouterSource extends object
```
Done
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 3 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Code-Review | +1 |
3 comments
NOTREACHED();Takashi NakayamaCan we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
Shunya ShishidoIt is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
Sounds good but then we don't need `NOTIMPLEMENTED()` I think.
yy-san's comment sounds like we should keep `NOTIMPLEMENTED()` to me.
CHECK(!or_condition.has_value() && !not_condition.has_value())Are we really sure that the router condition has neither `or_condition` nor `not_condition` here? If so who guarantees it? Since this causes the browser crash, we should carefully ensure that and clarify why it's safe.
CHECK(!or_condition.has_value() && !not_condition.has_value())
<< "nested conditions are not yet supported.";It might be good to leave TODO?
Both should be used in practice, and I believe having them are unavoidable to productinize this feature.
@otu...@google.com Could you add TODO comment like this?
```suggestion
// TODO(crbug.com/540469610): Support nested conditions.
CHECK(!or_condition.has_value() && !not_condition.has_value())
<< "nested conditions are not yet supported.";
- Keita Suzuki
- Shunya Shishido
- Sora Kobayashi
- Yoshisato Yanagisawa
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. |
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
NOTREACHED();Takashi NakayamaCan we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
Shunya ShishidoIt is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
Takashi NakayamaSounds good but then we don't need `NOTIMPLEMENTED()` I think.
yy-san's comment sounds like we should keep `NOTIMPLEMENTED()` to me.
If the intention is indeed to crash, NOTIMPLEMENTED() doesn't really serve any purpose here, and a TODO comment + NOTREACHED() would be cleaner. That said, this is a minor detail, so I'm fine either way.
CHECK(!or_condition.has_value() && !not_condition.has_value())Takashi NakayamaAre we really sure that the router condition has neither `or_condition` nor `not_condition` here? If so who guarantees it? Since this causes the browser crash, we should carefully ensure that and clarify why it's safe.
It is guaranteed [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_version.cc;l=2475;drc=c4e68511fe869c7aafd4f97d9a8fd4e5f052a8f3).
I'm happy with checking it on the caller side or leaving comments. As this CHECK is implicit in the callee side, I'm a bit worried about unexpected browser crashes in the future.
Related details
- Keita Suzuki
- Sora Kobayashi
- Yoshisato Yanagisawa
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
CHECK(!or_condition.has_value() && !not_condition.has_value())Takashi NakayamaAre we really sure that the router condition has neither `or_condition` nor `not_condition` here? If so who guarantees it? Since this causes the browser crash, we should carefully ensure that and clarify why it's safe.
Shunya ShishidoIt is guaranteed [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_version.cc;l=2475;drc=c4e68511fe869c7aafd4f97d9a8fd4e5f052a8f3).
I'm happy with checking it on the caller side or leaving comments. As this CHECK is implicit in the callee side, I'm a bit worried about unexpected browser crashes in the future.
Ack. @otu...@google.com Could you add a comment like this on the function declaration?
```
// Converts blink internal RouterCondition type to that of CDP.
// Note that nested conditions (`or` and `not`) are not supported on CDP.
// TODO(crbug.com/540469610): Support nested conditions.
```
CHECK(!version.router_rules);I failed to catch the meaning of this `CHECK`. It looks trivial upon L516, and I assume having `router_rules` can be used as a switch to go with the legacy implementation or the new implementation, but will the code after this may break if `version.router_rules` exist?
I don't think it's trivial. Rather, this CHECK validates the semantic invariant imposed on `ServiceWorkerVersion::typed_router_rules` [[cs]( https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_info.h;l=67;drc=71710b1d1f37409159fcaa69ff3f1ccd45862358)]. While we can simply branch by `if (version.router_rules) { ... } else if (!version.typed_router_rules.empty()) { ... }`, I'd like to keep the current code with `CHECK`.
Sora Kobayashi (Gerrit)
Sora Kobayashi added 3 comments![]( "Open in Gerrit")
CHECK(!or_condition.has_value() && !not_condition.has_value())Takashi NakayamaAre we really sure that the router condition has neither `or_condition` nor `not_condition` here? If so who guarantees it? Since this causes the browser crash, we should carefully ensure that and clarify why it's safe.
Shunya ShishidoIt is guaranteed [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_version.cc;l=2475;drc=c4e68511fe869c7aafd4f97d9a8fd4e5f052a8f3).
Takashi NakayamaI'm happy with checking it on the caller side or leaving comments. As this CHECK is implicit in the callee side, I'm a bit worried about unexpected browser crashes in the future.
Ack. @otu...@google.com Could you add a comment like this on the function declaration?
```
// Converts blink internal RouterCondition type to that of CDP.
// Note that nested conditions (`or` and `not`) are not supported on CDP.
// TODO(crbug.com/540469610): Support nested conditions.
```
Acknowledged
CHECK(!or_condition.has_value() && !not_condition.has_value())
<< "nested conditions are not yet supported.";Takashi NakayamaIt might be good to leave TODO?
Both should be used in practice, and I believe having them are unavoidable to productinize this feature.
@otu...@google.com Could you add TODO comment like this?
```suggestion
// TODO(crbug.com/540469610): Support nested conditions.
CHECK(!or_condition.has_value() && !not_condition.has_value())
<< "nested conditions are not yet supported.";
Sora Kobayashi```
Done
condition->SetRunningStatus("running");Please fix this WARNING reported by autoreview issue finding: For consistency with the rest of this file (see line 50), consider using the protocol constants: `protocol::ServiceWorker::ServiceWorkerVersionRunningStatusEnum::Running` and `protocol::ServiceWorker::ServiceWorkerVersionRunningStatusEnum::Stopped`.
- Keita Suzuki
- Shunya Shishido
- Takashi Nakayama
- Yoshisato Yanagisawa
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. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 1 comment![]( "Open in Gerrit")
CHECK(!version.router_rules);Takashi NakayamaI failed to catch the meaning of this `CHECK`. It looks trivial upon L516, and I assume having `router_rules` can be used as a switch to go with the legacy implementation or the new implementation, but will the code after this may break if `version.router_rules` exist?
I don't think it's trivial. Rather, this CHECK validates the semantic invariant imposed on `ServiceWorkerVersion::typed_router_rules` [[cs]( https://source.chromium.org/chromium/chromium/src/+/main:content/browser/service_worker/service_worker_info.h;l=67;drc=71710b1d1f37409159fcaa69ff3f1ccd45862358)]. While we can simply branch by `if (version.router_rules) { ... } else if (!version.typed_router_rules.empty()) { ... }`, I'd like to keep the current code with `CHECK`.
- Keita Suzuki
- Shunya Shishido
- Sora Kobayashi
- Takashi Nakayama
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Keita Suzuki
- Shunya Shishido
- Sora Kobayashi
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Keita Suzuki
- Shunya Shishido
- Sora Kobayashi
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. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
1 comment
NOTREACHED();Takashi NakayamaCan we just have `NOTREACHED()` to kRaceNetworkAndCache case instead of relying on fall through? Running through `NOTIMPLEMENTED()` then `NOTREACHED()` seems a bit strange.
Shunya ShishidoIt is possible that invalid inputs slip through all cases of switch. Though [the style guide](https://google.github.io/styleguide/cppguide.html#Switch_Statements) recommends preparing `default:` statement, this `NOTREACHED()` behaves identically.
Takashi NakayamaSounds good but then we don't need `NOTIMPLEMENTED()` I think.
Shunya Shishidoyy-san's comment sounds like we should keep `NOTIMPLEMENTED()` to me.
If the intention is indeed to crash, NOTIMPLEMENTED() doesn't really serve any purpose here, and a TODO comment + NOTREACHED() would be cleaner. That said, this is a minor detail, so I'm fine either way.
- Keita Suzuki
- Sora Kobayashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
NOTREACHED();`NOTIMPLEMENTED()` only logs a warning and does not return or abort execution. Execution will fall through to `NOTREACHED()`, which is fatal in Chromium and will crash the browser process.
Consider returning `nullptr` here instead, and then safely skipping this rule in `ConvertTypedRouterRules` so that DevTools doesn't crash the browser if a developer happens to have this internal feature enabled.
CHECK(!or_condition.has_value() && !not_condition.has_value())Using `CHECK` here is dangerous. If a web developer registers a Service Worker with nested conditions (e.g. `or` / `not`), this will crash the entire browser process when DevTools is opened.
Instead of crashing, you should handle this gracefully. For example, you could return `nullptr` here, and in `ConvertTypedRouterRules` skip rules that have an unsupported condition.
auto protocol_rule =If you change `ConvertRouterCondition` and `ConvertRouterSource` to return `nullptr` for unsupported features, you will need to assign them to local variables and check for null here. Since `condition` and `source` are required fields in the protocol, calling `.SetCondition(nullptr)` / `.SetSource(nullptr)` and then `.Build()` will fail.
For example:
```cpp
auto condition = ConvertRouterCondition(rule.condition);
auto source = ConvertRouterSource(rule.source);
if (!condition || !source) {
continue;
}
```
Related details
- Keita Suzuki
- Sora Kobayashi
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. |