[TabModel] Include index and pinned state in CreateTab [chromium/src : main]
0 views
Skip to first unread message
Dan Polanco (Gerrit)
Dec 15, 2025, 3:01:46 PM (yesterday) Dec 15
to Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Calder Kitagawa
Dan Polanco added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Calder Kitagawa
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Dec 15, 2025, 3:07:28 PM (yesterday) Dec 15
to Dan Polanco, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Dan Polanco
Calder Kitagawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
2 comments
Patchset-level comments
Calder Kitagawa . resolved
LGTM with request
File chrome/browser/android/persisted_tab_data/persisted_tab_data_android_browsertest.cc
Line 44, Patchset 6 (Latest):
tab_model->CreateTab(tab_android(), second_web_contents, /*index=*/-1,Calder Kitagawa . unresolved
Can we have a `TabModel::kInvalidIndex` constant for this rather than -1? Then you can also drop the `index` comment.
Related details
Attention is currently required from:
- Dan Polanco
Submit Requirements:
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. |
Dan Polanco (Gerrit)
Dec 15, 2025, 4:17:01 PM (23 hours ago) Dec 15
to Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Calder Kitagawa
Dan Polanco voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dan Polanco
| Commit-Queue | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Dan Polanco . resolved
Fixed TabStatsTrackerBrowserTest.
PTAL, thanks! (Lost +1)
File chrome/browser/android/persisted_tab_data/persisted_tab_data_android_browsertest.cc
Line 44, Patchset 6:
tab_model->CreateTab(tab_android(), second_web_contents, /*index=*/-1,Calder Kitagawa . resolved
Can we have a `TabModel::kInvalidIndex` constant for this rather than -1? Then you can also drop the `index` comment.
Attention is currently required from:
- Calder Kitagawa
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Polanco (Gerrit)
Dec 15, 2025, 4:17:38 PM (23 hours ago) Dec 15
to Darryl James, Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Calder Kitagawa and Darryl James
Dan Polanco added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Attention is currently required from:
- Calder Kitagawa
- Darryl James
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Dec 15, 2025, 4:19:40 PM (23 hours ago) Dec 15
to Dan Polanco, Darryl James, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Dan Polanco and Darryl James
Calder Kitagawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
1 comment
Patchset-level comments
Calder Kitagawa . resolved
LGTM
Attention is currently required from:
- Dan Polanco
- Darryl James
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Dec 15, 2025, 4:25:20 PM (23 hours ago) Dec 15
to Dan Polanco, Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Attention needed from Dan Polanco
Darryl James voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
2 comments
Patchset-level comments
Darryl James . resolved
lgtm!
File chrome/browser/ui/android/tab_model/tab_model.h
Line 47, Patchset 8 (Latest):
// Keep this in sync with
// chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java
static constexpr int kInvalidIndex = -1;Darryl James . unresolved
optional: Not sure if this will apply into android targets but if `LINT` guards are available I recommend using them in this case. That way the presubmit warnings will automatically flag changes in the future.
Ex:
```
// LINT.IfChange(kInvalidIndex)
static constexpr int kInvalidIndex = -1;
// LINT.ThenChange(//chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java)
```
Related details
Attention is currently required from:
- Dan Polanco
Submit Requirements:
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. |
Dan Polanco (Gerrit)
Dec 15, 2025, 6:00:46 PM (21 hours ago) Dec 15
to Darryl James, Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Dan Polanco voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dan Polanco
| Commit-Queue | +2 |
1 comment
File chrome/browser/ui/android/tab_model/tab_model.h
Line 47, Patchset 8:
// Keep this in sync with
// chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java
static constexpr int kInvalidIndex = -1;Darryl James . resolved
optional: Not sure if this will apply into android targets but if `LINT` guards are available I recommend using them in this case. That way the presubmit warnings will automatically flag changes in the future.
Ex:
```
// LINT.IfChange(kInvalidIndex)static constexpr int kInvalidIndex = -1;
// LINT.ThenChange(//chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java)
```
Dan Polanco
Thanks! Looking at clank files, I think they are
Done
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. |
Dan Polanco (Gerrit)
Dec 15, 2025, 8:17:06 PM (19 hours ago) Dec 15
to Darryl James, Calder Kitagawa, Chromium LUCI CQ, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Dan Polanco voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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. |
Chromium LUCI CQ (Gerrit)
Dec 15, 2025, 11:45:37 PM (16 hours ago) Dec 15
to Dan Polanco, Darryl James, Calder Kitagawa, AyeAye, chrome-intelligence-te...@google.com, msrame...@chromium.org, feature-v...@chromium.org, devtools...@chromium.org, mfoltz+wa...@chromium.org, asvitki...@chromium.org, chrome-intell...@chromium.org, extension...@chromium.org, chromiumme...@microsoft.com, chromium-a...@chromium.org, dullweb...@chromium.org, ntp-dev...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
8 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/android/tab_model/tab_model.h
Insertions: 2, Deletions: 0.
@@ -44,9 +44,11 @@
public:
DECLARE_USER_DATA(TabModel);
+ // LINT.IfChange(kInvalidIndex)
// Keep this in sync with
// chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java
static constexpr int kInvalidIndex = -1;
+ // LINT.ThenChange(//chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java:INVALID_TAB_INDEX)
// LINT.IfChange(TabLaunchType)
// Various ways tabs can be launched.
```
```
The name of the file: chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java
Insertions: 3, Deletions: 0.
@@ -16,9 +16,12 @@
@MockedInTests // Needed due to R8's computeDelayedInterfaceMethodSyntheticBridges. b/147584922
@NullMarked
public interface TabList extends Iterable<Tab> {
+ // LINT.IfChange(INVALID_TAB_INDEX)
// Keep this in sync with chrome/browser/ui/android/tab_model/tab_model.h
int INVALID_TAB_INDEX = -1;
+ // LINT.ThenChange(//chrome/browser/ui/android/tab_model/tab_model.h:kInvalidIndex)
+
/**
* TODO(crbug.com/350654700): clean up usages and remove isIncognito.
*
```
Change information
Commit message:
[TabModel] Include index and pinned state in CreateTab
This adds index and pinned state in the TabModel#CreateTab signature
(C++). Follow-up CL actually implements this on Java.
Bug: 466333033
Change-Id: Ib655ee6bd0000123c0442fdd173c35c282708d9b
Reviewed-by: Darryl James <dlj...@chromium.org>
Commit-Queue: Dan Polanco <pol...@google.com>
Reviewed-by: Calder Kitagawa <ckit...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1559126}
Files:
- M chrome/browser/android/historical_tab_saver.cc
- M chrome/browser/android/persisted_tab_data/persisted_tab_data_android_browsertest.cc
- M chrome/browser/auxiliary_search/auxiliary_search_provider_browsertest.cc
- M chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager_browsertest.cc
- M chrome/browser/devtools/devtools_window.cc
- M chrome/browser/extensions/api/identity/web_auth_flow.cc
- M chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
- M chrome/browser/extensions/api/tabs/tabs_api_android.cc
- M chrome/browser/metrics/tab_stats/tab_stats_tracker_browsertest.cc
- M chrome/browser/page_content_annotations/android/page_content_cache_android_browsertest.cc
- M chrome/browser/sessions/session_restore_android.cc
- M chrome/browser/tabmodel/android/java/src/org/chromium/chrome/browser/tabmodel/TabList.java
- M chrome/browser/ui/android/tab_model/android_live_tab_context.cc
- M chrome/browser/ui/android/tab_model/tab_model.cc
- M chrome/browser/ui/android/tab_model/tab_model.h
- M chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
- M chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h
- M chrome/browser/ui/android/tab_model/tab_model_test_helper.cc
- M chrome/browser/ui/android/tab_model/tab_model_test_helper.h
- M chrome/browser/visited_url_ranking/group_suggestions_service_factory_browsertest.cc
- M chrome/browser/vr/test/xr_browser_test.cc
- M chrome/test/base/android/android_ui_test_utils.cc
Change size: M
Delta: 22 files changed, 74 insertions(+), 33 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Darryl James, +1 by Calder Kitagawa
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages