[GLIC] Include experimental_triggering opt-in state in DeviceInfo [chromium/src : main]
0 views
Skip to first unread message
Theodore Olsauskas-Warren (Gerrit)
Apr 22, 2026, 8:16:14 PM (22 hours ago) Apr 22
to Mikel Astiz, Tommy Nyquist, Susi Afrianita, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-r...@chromium.org, ios-revie...@chromium.org, ios-web-view...@google.com, ios-rev...@chromium.org, marq+...@chromium.org, dewitt...@chromium.org, dfried...@chromium.org, derinel+wat...@google.com, estali...@chromium.org, webauthn...@chromium.org, oshima...@chromium.org, knollr+wat...@chromium.org, chrome-tab-group-en...@google.com, jeffreycohen+watc...@chromium.org, tgupta...@chromium.org
Attention needed from Mikel Astiz and Tommy Nyquist
Theodore Olsauskas-Warren added 3 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 13 (Latest):
Theodore Olsauskas-Warren . unresolved
@mas...@chromium.org - My (heavily jetski assisted) attempt at adding to the DeviceInfo proto. I didn't realize quite how painful touching the constructor would be, and I don't have an iOS build to test the failures so its doubly painful.
Does this seem like the right direction to you?
File chrome/browser/ui/views/user_education/ios_promo_bubble_view_unittest.cc
Line 60, Patchset 13 (Latest):
"guid", "iPhone", "Chrome 100", "User Agent",
syncer::DeviceInfo::DeviceType::kPhone, syncer::DeviceInfo::OsType::kIOS,
syncer::DeviceInfo::FormFactor::kPhone, "device_id", "manufacturer",
"model", "full_hardware_class", base::Time::Now(), base::TimeDelta(),Theodore Olsauskas-Warren . unresolved
I will revert this. Gemini getting excited.
File ios/chrome/browser/synced_set_up/coordinator/DEPS
Line 8, Patchset 13 (Latest):
specific_include_rules = {
".*unittest\\.mm": [
"+components/desktop_to_mobile_promos/features.h",
],
}Theodore Olsauskas-Warren . unresolved
I will have to figure out how to unwind this in a much less ugly way. Any tips much appreciated!
Related details
Attention is currently required from:
- Mikel Astiz
- Tommy Nyquist
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. |
Theodore Olsauskas-Warren (Gerrit)
Apr 22, 2026, 8:17:07 PM (22 hours ago) Apr 22
to Mikel Astiz, Tommy Nyquist, Susi Afrianita, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-r...@chromium.org, ios-revie...@chromium.org, ios-web-view...@google.com, ios-rev...@chromium.org, marq+...@chromium.org, dewitt...@chromium.org, dfried...@chromium.org, derinel+wat...@google.com, estali...@chromium.org, webauthn...@chromium.org, oshima...@chromium.org, knollr+wat...@chromium.org, chrome-tab-group-en...@google.com, jeffreycohen+watc...@chromium.org, tgupta...@chromium.org
Attention needed from Mikel Astiz and Tommy Nyquist
Theodore Olsauskas-Warren added 1 comment![]( "Open in Gerrit")
File components/sync/protocol/device_info_specifics.proto
Line 132, Patchset 13 (Latest):
// Tracks whether the device has been opted into experimental triggering.
optional bool experimental_triggering_opted_in = 6;Theodore Olsauskas-Warren . unresolved
I simplified this down to a bool instead of an ENUM, I think the semantics are clearer and we can avoid doing anything on the server that required a more nuanced distiction.
Mikel Astiz (Gerrit)
1:23 PM (5 hours ago) 1:23 PM
to Theodore Olsauskas-Warren, Tommy Nyquist, Susi Afrianita, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org, ios-r...@chromium.org, ios-revie...@chromium.org, ios-web-view...@google.com, ios-rev...@chromium.org, marq+...@chromium.org, dewitt...@chromium.org, dfried...@chromium.org, derinel+wat...@google.com, estali...@chromium.org, webauthn...@chromium.org, oshima...@chromium.org, knollr+wat...@chromium.org, chrome-tab-group-en...@google.com, jeffreycohen+watc...@chromium.org, tgupta...@chromium.org
Attention needed from Theodore Olsauskas-Warren and Tommy Nyquist
Mikel Astiz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Code-Review | +1 |
4 comments
Patchset-level comments
Mikel Astiz . resolved
LGTM.
File chrome/browser/sync/device_info_sync_client_impl.h
Line 59, Patchset 13 (Latest):
bool GetExperimentalTriggeringOptedIn() const override;Mikel Astiz . unresolved
Here and throughout this patch (proto field, variables names etc.): can we add something like `glic` to this name to make it more specific? I mean in Sync code, where the filename or namespace doesn't already imply this.
File chrome/browser/ui/views/user_education/ios_promo_bubble_view_unittest.cc
Line 60, Patchset 13 (Latest):
"guid", "iPhone", "Chrome 100", "User Agent",
syncer::DeviceInfo::DeviceType::kPhone, syncer::DeviceInfo::OsType::kIOS,
syncer::DeviceInfo::FormFactor::kPhone, "device_id", "manufacturer",
"model", "full_hardware_class", base::Time::Now(), base::TimeDelta(),Theodore Olsauskas-Warren . unresolved
Mikel AstizI will revert this. Gemini getting excited.
Acknowledged
File components/sync/protocol/device_info_specifics.proto
Line 132, Patchset 13 (Latest):
// Tracks whether the device has been opted into experimental triggering.
optional bool experimental_triggering_opted_in = 6;Theodore Olsauskas-Warren . unresolved
I simplified this down to a bool instead of an ENUM, I think the semantics are clearer and we can avoid doing anything on the server that required a more nuanced distiction.
Attention is currently required from:
- Theodore Olsauskas-Warren
- Tommy Nyquist
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. |
Reply all
Reply to author
Forward
0 new messages