Introduce a validation framework for tab strip service [chromium/src : main]
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
Related details
- David Yeung
- Keren Zhu
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu added 2 comments![]( "Open in Gerrit")
#include "base/types/expected_macros.h"TIL `RETURN_IF_ERROR` and `ASSIGN_OR_RETURN`. Looks useful.
ASSIGN_OR_RETURN(auto tab_id, validations::GetNativeId(id));It is a bit weird that `GetNativeId()` is under `validation` namespace. I think the `NodeId` -> `NativeId` conversion deserves a helper function (I almost made one myself), but I won't search under the `./validation` folder for such a helper.
WDYT of simply adding the 2 helper functions under the `./utilities` folder?
Related details
- David Yeung
- Fred Shih
- Tom Lukaszewicz
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. |
David Yeung (Gerrit)
David Yeung added 3 comments![]( "Open in Gerrit")
RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));nit. I think this makes it harder to read. Curious what your thoughts are about readability
if (tab_id.Type() != tabs_api::NodeId::Type::kContent) {
return base::unexpected(
mojo_base::mojom::Error::New(mojo_base::mojom::Code::kInvalidArgument,
"only a content tab id can be provided"));
}There are several places where we'd want to change to `RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));`. Is that a follow up CL?
ASSIGN_OR_RETURN(auto handle_id, validations::GetNativeId(tab_id));Can we introduce a validation framework one level above the service implementation? Maybe a centralized place whenever a NodeId is forwarded over rather than having to check every time the api is used.
Related details
- Fred Shih
- Tom Lukaszewicz
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. |
Fred Shih (Gerrit)
Fred Shih added 4 comments![]( "Open in Gerrit")
RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));nit. I think this makes it harder to read. Curious what your thoughts are about readability
I didn't like it at first when I used this pattern in Google3, but I have sorta come around to it. It's more of a side-effect of us not being able to throw exceptions IMO.
This CL (and the macro lib) are based on Google3's macros:
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/labs/fantastic-macros.md?cl=head
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/121.md?cl=head
I do admit that it looks kinda bleh at first though... Is there any other pattern you'd prefer?
I also changed the function to `CheckIsContentType` to make it more clear that this is performing some sort of validation. I can see how it's confusing that a `IsContentType` would end up returning an error..
ASSIGN_OR_RETURN(auto tab_id, validations::GetNativeId(id));It is a bit weird that `GetNativeId()` is under `validation` namespace. I think the `NodeId` -> `NativeId` conversion deserves a helper function (I almost made one myself), but I won't search under the `./validation` folder for such a helper.
WDYT of simply adding the 2 helper functions under the `./utilities` folder?
I am not super opinionated on this. I had originally placed it under validations because it returns a InvalidArgument error, but I could also see the merit of putting it under utilities. If this makes more sense to you, then I will move it over there. Done!
if (tab_id.Type() != tabs_api::NodeId::Type::kContent) {
return base::unexpected(
mojo_base::mojom::Error::New(mojo_base::mojom::Code::kInvalidArgument,
"only a content tab id can be provided"));
}There are several places where we'd want to change to `RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));`. Is that a follow up CL?
yep, followup later. This is more of a demonstration on the new way of checking-for and propagating errors.
ASSIGN_OR_RETURN(auto handle_id, validations::GetNativeId(tab_id));Can we introduce a validation framework one level above the service implementation? Maybe a centralized place whenever a NodeId is forwarded over rather than having to check every time the api is used.
I'm not sure how we would do this in a generic way. The node id can be embedded as part of a request, for example, as part of a Tab object. In that scenario, the interceptor (?) would have to have contextual knowledge about the underlying resource to perform meaningful validation.
The only other way I can think of is to have manual validation written for each request type (and probably for each method). For example, when passing in NodeId, some services require that a specific type of node id is passed in, how would we declare that at higher level?
Related details
- David Yeung
- Keren Zhu
- Tom Lukaszewicz
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. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 1 comment![]( "Open in Gerrit")
- David Yeung
- Fred Shih
- Tom Lukaszewicz
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. |
David Yeung (Gerrit)
David Yeung voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Yeung
| Code-Review | +1 |
3 comments
RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));Fred Shihnit. I think this makes it harder to read. Curious what your thoughts are about readability
I didn't like it at first when I used this pattern in Google3, but I have sorta come around to it. It's more of a side-effect of us not being able to throw exceptions IMO.
This CL (and the macro lib) are based on Google3's macros:
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/labs/fantastic-macros.md?cl=head
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/121.md?cl=headI do admit that it looks kinda bleh at first though... Is there any other pattern you'd prefer?
I also changed the function to `CheckIsContentType` to make it more clear that this is performing some sort of validation. I can see how it's confusing that a `IsContentType` would end up returning an error..
I just assumed any macro that hides errors end up as an anti-pattern because its harder to read and debug. By centralizing these validation checks, I'm worried we may also end up having one-off validation checks and just explode the validation file.
Happy to try this out though and be proven wrong.
ASSIGN_OR_RETURN(auto handle_id, validations::GetNativeId(tab_id));Fred ShihCan we introduce a validation framework one level above the service implementation? Maybe a centralized place whenever a NodeId is forwarded over rather than having to check every time the api is used.
I'm not sure how we would do this in a generic way. The node id can be embedded as part of a request, for example, as part of a Tab object. In that scenario, the interceptor (?) would have to have contextual knowledge about the underlying resource to perform meaningful validation.
The only other way I can think of is to have manual validation written for each request type (and probably for each method). For example, when passing in NodeId, some services require that a specific type of node id is passed in, how would we declare that at higher level?
I don't have any suggestions. The main downside I had was that each API would need to remember to add a validation check.
base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeId(Should this be explicit for tab ids? GetNativeTabId?
Related details
- Fred Shih
- Tom Lukaszewicz
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 2 comments![]( "Open in Gerrit")
RETURN_IF_ERROR(validations::IsContentType(tab_mojom_id));Fred Shihnit. I think this makes it harder to read. Curious what your thoughts are about readability
David YeungI didn't like it at first when I used this pattern in Google3, but I have sorta come around to it. It's more of a side-effect of us not being able to throw exceptions IMO.
This CL (and the macro lib) are based on Google3's macros:
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/labs/fantastic-macros.md?cl=head
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/121.md?cl=headI do admit that it looks kinda bleh at first though... Is there any other pattern you'd prefer?
I also changed the function to `CheckIsContentType` to make it more clear that this is performing some sort of validation. I can see how it's confusing that a `IsContentType` would end up returning an error..
I just assumed any macro that hides errors end up as an anti-pattern because its harder to read and debug. By centralizing these validation checks, I'm worried we may also end up having one-off validation checks and just explode the validation file.
Happy to try this out though and be proven wrong.
we can always create more files :)
base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeId(Should this be explicit for tab ids? GetNativeTabId?
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.cc
Insertions: 1, Deletions: 1.
@@ -18,7 +18,7 @@
return base::ok();
}
-base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeId(
+base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeTabId(
const NodeId& node_id) {
int32_t tab_id;
if (!base::StringToInt(node_id.Id(), &tab_id)) {
```
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils_unittest.cc
Insertions: 4, Deletions: 4.
@@ -20,14 +20,14 @@
ASSERT_EQ(mojo_base::mojom::Code::kInvalidArgument, result.error()->code);
}
-TEST(TabStripApiUtilsTest, GetNativeId) {
- auto result = GetNativeId(NodeId(NodeId::Type::kContent, "123"));
+TEST(TabStripApiUtilsTest, GetNativeTabId) {
+ auto result = GetNativeTabId(NodeId(NodeId::Type::kContent, "123"));
ASSERT_TRUE(result.has_value());
ASSERT_EQ(123, result.value());
}
-TEST(TabStripApiUtilsTest, GetNativeId_BadType) {
- auto result = GetNativeId(NodeId(NodeId::Type::kContent, "abc"));
+TEST(TabStripApiUtilsTest, GetNativeTabId_BadType) {
+ auto result = GetNativeTabId(NodeId(NodeId::Type::kContent, "abc"));
ASSERT_FALSE(result.has_value());
ASSERT_EQ(mojo_base::mojom::Code::kInvalidArgument, result.error()->code);
}
```
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.h
Insertions: 1, Deletions: 1.
@@ -13,7 +13,7 @@
base::expected<void, mojo_base::mojom::ErrorPtr> CheckIsContentType(
const NodeId& node_id);
-base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeId(
+base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetNativeTabId(
const NodeId& node_id);
} // namespace tabs_api::utils
```
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_impl.cc
Insertions: 3, Deletions: 3.
@@ -121,7 +121,7 @@
auto session = session_controller_->CreateSession();
RETURN_IF_ERROR(utils::CheckIsContentType(tab_mojom_id));
- ASSIGN_OR_RETURN(auto tab_id, utils::GetNativeId(tab_mojom_id));
+ ASSIGN_OR_RETURN(auto tab_id, utils::GetNativeTabId(tab_mojom_id));
tabs_api::mojom::TabPtr tab_result;
// TODO (crbug.com/412709270) TabStripModel or TabCollections should have an
@@ -237,7 +237,7 @@
"only a content tab id can be provided"));
}
- ASSIGN_OR_RETURN(auto tab_id, utils::GetNativeId(id));
+ ASSIGN_OR_RETURN(auto tab_id, utils::GetNativeTabId(id));
auto maybe_idx =
tab_strip_model_adapter_->GetIndexForHandle(tabs::TabHandle(tab_id));
@@ -378,7 +378,7 @@
"only a content tab id can be provided"));
}
- ASSIGN_OR_RETURN(auto handle_id, utils::GetNativeId(tab_id));
+ ASSIGN_OR_RETURN(auto handle_id, utils::GetNativeTabId(tab_id));
auto maybe_idx =
tab_strip_model_adapter_->GetIndexForHandle(tabs::TabHandle(handle_id));
```
Change information
Introduce a validation framework for tab strip service
There are a lot of validations for tab strip service, which can make it
difficult to test the validation logic. To make matters worse, most of
the methods copy-and-paste the validation logic throughout the service.
Instead, group the validation in a central place and make each validator
logically simple. Use the expected macros to take advantage our usage of
base::expected. This allows us to easily short-circuit return or
assign-or-return on validation.
Unit testing for these validations are very simple because they are
stateless function calls.
There is no need to test the service methods themselves because they
validations are performed inline and falls straight through. The assign
or return ensures that we do not use unvalidated variables in our code.
This will not cover all cases. There might be some methods which require
very specific validations. Those methods will need to explicitly test
those validations in their unit_test.
- M chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_impl.cc
- M chrome/browser/ui/tabs/tab_strip_api/utilities/BUILD.gn
- A chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.cc
- A chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.h
- A chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils_unittest.cc
Code-Review: +1 by Keren Zhu, +1 by David Yeung
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |