Keren Zhu (Gerrit)

unread,

Dec 23, 2025, 9:25:33 PM (3 days ago) Dec 23

to Fred Shih, David Yeung, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Yeung and Fred Shih

Keren Zhu voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

David Yeung
Fred Shih

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

David Yeung (Gerrit)

unread,

1:39 PM (10 hours ago) 1:39 PM

to Fred Shih, Keren Zhu, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Fred Shih

David Yeung voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Fred Shih

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Fred Shih (Gerrit)

unread,

7:43 PM (3 hours ago) 7:43 PM

to David Yeung, Keren Zhu, Chromium LUCI CQ, chromium...@chromium.org

Fred Shih voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

8:34 PM (3 hours ago) 8:34 PM

to Fred Shih, David Yeung, Keren Zhu, chromium...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

1 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_unittest.cc
Insertions: 4, Deletions: 4.

@@ -20,26 +20,26 @@
   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);
 }
 
-TEST(TabStripApiUtilsTest, GetContentNativeId) {
-  auto result = GetContentNativeId(NodeId(NodeId::Type::kContent, "123"));
+TEST(TabStripApiUtilsTest, GetContentNativeTabId) {
+  auto result = GetContentNativeTabId(NodeId(NodeId::Type::kContent, "123"));
   ASSERT_TRUE(result.has_value());
   ASSERT_EQ(123, result.value());
 }
 
-TEST(TabStripApiUtilsTest, GetContentNativeId_Invalid) {
-  auto result = GetContentNativeId(NodeId(NodeId::Type::kInvalid, "123"));
+TEST(TabStripApiUtilsTest, GetContentNativeTabId_Invalid) {
+  auto result = GetContentNativeTabId(NodeId(NodeId::Type::kInvalid, "123"));
   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.cc
Insertions: 2, Deletions: 2.

@@ -19,7 +19,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)) {
@@ -29,10 +29,10 @@
   return tab_id;
 }
 
-base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetContentNativeId(
+base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetContentNativeTabId(
     const NodeId& node_id) {
   RETURN_IF_ERROR(CheckIsContentType(node_id));
-  ASSIGN_OR_RETURN(auto native, GetNativeId(node_id));
+  ASSIGN_OR_RETURN(auto native, GetNativeTabId(node_id));
   return native;
 }
 
```
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_impl.cc
Insertions: 8, Deletions: 20.

@@ -120,7 +120,7 @@
     const tabs_api::NodeId& tab_mojom_id) {
   auto session = session_controller_->CreateSession();
 
-  ASSIGN_OR_RETURN(auto tab_id, utils::GetContentNativeId(tab_mojom_id));
+  ASSIGN_OR_RETURN(auto tab_id, utils::GetContentNativeTabId(tab_mojom_id));
 
   tabs_api::mojom::TabPtr tab_result;
   // TODO (crbug.com/412709270) TabStripModel or TabCollections should have an
@@ -191,13 +191,8 @@
 
   std::vector<int32_t> tab_content_targets;
   for (const auto& id : ids) {
-    if (id.Type() != tabs_api::NodeId::Type::kContent) {
-      return base::unexpected(mojo_base::mojom::Error::New(
-          mojo_base::mojom::Code::kUnimplemented,
-          "only content tab closing has been implemented right now"));
-    }
-    ASSIGN_OR_RETURN(int32_t numeric_id, utils::GetNativeId(id));
-    tab_content_targets.push_back(numeric_id);
+    ASSIGN_OR_RETURN(auto content_id, utils::GetContentNativeTabId(id));
+    tab_content_targets.push_back(content_id);
   }
 
   std::vector<size_t> tab_strip_indices;
@@ -226,7 +221,7 @@
     const tabs_api::NodeId& id) {
   auto session = session_controller_->CreateSession();
 
-  ASSIGN_OR_RETURN(auto tab_id, utils::GetContentNativeId(id));
+  ASSIGN_OR_RETURN(auto tab_id, utils::GetContentNativeTabId(id));
 
   auto maybe_idx =
       tab_strip_model_adapter_->GetIndexForHandle(tabs::TabHandle(tab_id));
@@ -264,19 +259,12 @@
 
   std::vector<tabs::TabHandle> selection_handles;
   for (auto& id : selection) {
-    int32_t handle_id;
-    if (!base::StringToInt(id.Id(), &handle_id)) {
-      return base::unexpected(mojo_base::mojom::Error::New(
-          mojo_base::mojom::Code::kInvalidArgument, "id is malformed"));
-    }
+    ASSIGN_OR_RETURN(auto handle_id, utils::GetNativeTabId(id));
     selection_handles.push_back(tabs::TabHandle(handle_id));
   }
 
-  int32_t activate_handle_id;
-  if (!base::StringToInt(tab_to_activate.Id(), &activate_handle_id)) {
-    return base::unexpected(mojo_base::mojom::Error::New(
-        mojo_base::mojom::Code::kInvalidArgument, "activate id is malformed"));
-  }
+  ASSIGN_OR_RETURN(auto activate_handle_id,
+                   utils::GetNativeTabId(tab_to_activate));
 
   tab_strip_model_adapter_->SetTabSelection(
       selection_handles, tabs::TabHandle(activate_handle_id));
@@ -361,7 +349,7 @@
                                         const gfx::Point& location) {
   auto session = session_controller_->CreateSession();
 
-  ASSIGN_OR_RETURN(auto handle_id, utils::GetContentNativeId(tab_id));
+  ASSIGN_OR_RETURN(auto handle_id, utils::GetContentNativeTabId(tab_id));
 
   auto maybe_idx =
       tab_strip_model_adapter_->GetIndexForHandle(tabs::TabHandle(handle_id));
```
```
The name of the file: chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.h
Insertions: 1, Deletions: 1.

@@ -13,12 +13,12 @@
 
 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);
 
 // Gets the native id for a content tab. Error if the id is not for a content
 // type.
-base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetContentNativeId(
+base::expected<int32_t, mojo_base::mojom::ErrorPtr> GetContentNativeTabId(
     const NodeId& node_id);
 
 }  // namespace tabs_api::utils
```

Change information

Commit message:

Move more things over to the new validation pattern

Introduce a new util to get the content native id, because it's very
common for methods to require that the id passed in is a content tab id.

Removed unittests that are no longer necessary. Reading the code clearly
indicates whether or not a validation is performed.

Change-Id: Ic9bfe39246ef4bf70618dee0703d9d71c240f7a8

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7308942

Reviewed-by: Keren Zhu <kere...@chromium.org>

Reviewed-by: David Yeung <day...@chromium.org>

Commit-Queue: Fred Shih <ff...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1562825}

Files:

M chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_impl.cc
M chrome/browser/ui/tabs/tab_strip_api/tab_strip_service_impl_unittest.cc
M chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.cc
M chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils.h
M chrome/browser/ui/tabs/tab_strip_api/utilities/tab_id_utils_unittest.cc

Change size: M

Delta: 5 files changed, 32 insertions(+), 79 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Keren Zhu, +1 by David Yeung

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Move more things over to the new validation pattern [chromium/src : main]

Keren Zhu (Gerrit)

Keren Zhu voted Code-Review+1

Related details

David Yeung (Gerrit)

David Yeung voted Code-Review+1

Related details

Fred Shih (Gerrit)

Fred Shih voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information