[glic] Add GetSendTabToSelfDevices API and tests [chromium/src : main]
Jeffrey Cohen (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Bryant Chandler
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeffrey Cohen (Gerrit)
Jeffrey Cohen voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Bryant Chandler
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mpde...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mpde...@chromium.org
Reviewer source(s):
mpde...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Bryant Chandler
- Matthew Denton
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Bryant Chandler
- Jeffrey Cohen
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bryant Chandler (Gerrit)
Bryant Chandler added 3 comments on change 8613532![]( "Open in Gerrit")
enum SendTabToSelfFormFactor {
[Default] kUnknown = 0,
kDesktop = 1,
kPhone = 2,
kTablet = 3,
};
We can reuse `FormFactor` from `chrome/common/glic_enums.mojom` here instead of adding a duplicate enum—it already has the exact same values and is exported in `glic_api.ts`.
struct SendTabToSelfDevice {
string cache_guid;
string device_name;
SendTabToSelfFormFactor form_factor;
double last_updated_timestamp_ms;
};
A few things on this struct:
1. Can we use `mojo_base.mojom.Time` for `last_updated_timestamp` instead of `double last_updated_timestamp_ms`? `generate.py` already maps `mojo_base.mojom.Time` to `Date` and `conversions.ts` has `timeToClient()` (matching `SkillPreview.creation_time`).
2. Probably name `cache_guid` `device_guid` (or `guid`) so we don't leak Sync cache terminology to the web client and it aligns better with `target_device_guid` in https://crrev.com/c/8614040.
3. Please add doc comments to the struct and fields so `generate.py` carries them over to `glic_api_generated.ts`.
auto device = mojom::SendTabToSelfDevice::New();
device->cache_guid = info.cache_guid;
device->device_name = info.device_name;
switch (info.form_factor) {
case syncer::DeviceInfo::FormFactor::kDesktop:
device->form_factor = mojom::SendTabToSelfFormFactor::kDesktop;
break;
case syncer::DeviceInfo::FormFactor::kPhone:
device->form_factor = mojom::SendTabToSelfFormFactor::kPhone;
break;
case syncer::DeviceInfo::FormFactor::kTablet:
device->form_factor = mojom::SendTabToSelfFormFactor::kTablet;
break;
default:
device->form_factor = mojom::SendTabToSelfFormFactor::kUnknown;
break;
}
device->last_updated_timestamp_ms =
info.last_updated_timestamp.InMillisecondsFSinceUnixEpoch();`devices` is returned by value, so we can iterate by non-const reference and `std::move` `info.cache_guid` and `info.device_name`.
Also, if we keep the `double` timestamp instead of switching to `mojo_base.mojom.Time`, please use `InMillisecondsFSinceUnixEpochIgnoringNull()` here—`base/time/time.h` discourages `InMillisecondsFSinceUnixEpoch()` in new code.
Related details
- Jeffrey Cohen
- Trevor Perrier
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. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1 on change 8613532![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jeffrey Cohen
- Trevor Perrier
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. |
Jeffrey Cohen (Gerrit)
Jeffrey Cohen added 4 comments on change 8613532![]( "Open in Gerrit")
@bryantchandler: heads-up on one extra change in PS6. The new GEiC presubmit check ([crrev.com/c/8503368](https://crrev.com/c/8503368)) flags the new `ApiRequestType` variant because `Glic.Api.RequestCounts.{ApiRequestType}` / `Glic.Api.RequestHostLatency.{ApiRequestType}` aren't split by `{GlicProvider}`. This CL adds an `<!-- Allowed unsplittable: ... -->` comment to those two shared histograms in `tools/metrics/histograms/metadata/glic/histograms.xml`, with reason text matching the existing "Not split" comment in `glic_web_client_handler.cc` (added in [crrev.com/c/8503302](https://crrev.com/c/8503302)). Happy to switch to a different approach (e.g. migrating these to `GlicMetricReporter` in a separate CL) if you and austinzzr@ prefer.
enum SendTabToSelfFormFactor {
[Default] kUnknown = 0,
kDesktop = 1,
kPhone = 2,
kTablet = 3,
};
We can reuse `FormFactor` from `chrome/common/glic_enums.mojom` here instead of adding a duplicate enum—it already has the exact same values and is exported in `glic_api.ts`.
Done. Reused `FormFactor` from `glic_enums.mojom`; the handler still maps `syncer::DeviceInfo::FormFactor` via an explicit `switch` with `default` → `kUnknown`, so sync-only values (`kAutomotive`, `kWearable`, `kTv`) degrade safely and the two enums needn't stay in lockstep.
struct SendTabToSelfDevice {
string cache_guid;
string device_name;
SendTabToSelfFormFactor form_factor;
double last_updated_timestamp_ms;
};
A few things on this struct:
1. Can we use `mojo_base.mojom.Time` for `last_updated_timestamp` instead of `double last_updated_timestamp_ms`? `generate.py` already maps `mojo_base.mojom.Time` to `Date` and `conversions.ts` has `timeToClient()` (matching `SkillPreview.creation_time`).
2. Probably name `cache_guid` `device_guid` (or `guid`) so we don't leak Sync cache terminology to the web client and it aligns better with `target_device_guid` in https://crrev.com/c/8614040.
3. Please add doc comments to the struct and fields so `generate.py` carries them over to `glic_api_generated.ts`.
Done. (1) Now `mojo_base.mojom.Time? last_updated_time` (nullable, matching `SkillPreview.creation_time`), converted with `timeToClient()` to `lastUpdatedTime?: Date`. (2) Renamed `cache_guid` → `device_guid`. (3) Added doc comments on the struct and fields; regenerated `glic_api_generated.ts` with `generate.py`.
auto device = mojom::SendTabToSelfDevice::New();
device->cache_guid = info.cache_guid;
device->device_name = info.device_name;
switch (info.form_factor) {
case syncer::DeviceInfo::FormFactor::kDesktop:
device->form_factor = mojom::SendTabToSelfFormFactor::kDesktop;
break;
case syncer::DeviceInfo::FormFactor::kPhone:
device->form_factor = mojom::SendTabToSelfFormFactor::kPhone;
break;
case syncer::DeviceInfo::FormFactor::kTablet:
device->form_factor = mojom::SendTabToSelfFormFactor::kTablet;
break;
default:
device->form_factor = mojom::SendTabToSelfFormFactor::kUnknown;
break;
}
device->last_updated_timestamp_ms =
info.last_updated_timestamp.InMillisecondsFSinceUnixEpoch();`devices` is returned by value, so we can iterate by non-const reference and `std::move` `info.cache_guid` and `info.device_name`.
Also, if we keep the `double` timestamp instead of switching to `mojo_base.mojom.Time`, please use `InMillisecondsFSinceUnixEpochIgnoringNull()` here—`base/time/time.h` discourages `InMillisecondsFSinceUnixEpoch()` in new code.
Done. Iterating by non-const reference and `std::move`-ing `cache_guid` / `device_name`; the `base::Time` is now assigned directly to the mojo `Time` field.
Related details
- Bryant Chandler
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeffrey Cohen (Gerrit)
Jeffrey Cohen voted Commit-Queue+1 on change 8613532![]( "Open in Gerrit")
| Commit-Queue | +1 |
Bryant Chandler (Gerrit)
Bryant Chandler voted Code-Review+1![]( "Open in Gerrit")
- Jeffrey Cohen
- Trevor Perrier
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Trevor Perrier (Gerrit)
Trevor Perrier voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jeffrey Cohen
Jeffrey Cohen (Gerrit)
Jeffrey Cohen voted Commit-Queue+2![]( "Open in Gerrit")
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![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[glic] Add GetSendTabToSelfDevices API and tests
Adds GetSendTabToSelfDevices to WebClientHandler mojom, GlicBrowserHost
TS API, metrics, and unit/browser tests.
- M chrome/browser/glic/BUILD.gn
- M chrome/browser/glic/host/glic.mojom
- M chrome/browser/glic/host/glic_api_browsertest.cc
- M chrome/browser/glic/host/glic_features.mojom
- M chrome/browser/glic/host/glic_web_client_handler.cc
- M chrome/browser/glic/host/guest_util.cc
- M chrome/browser/glic/host/guest_util_unittest.cc
- M chrome/browser/glic/public/features.cc
- M chrome/browser/glic/public/features.h
- M chrome/browser/glic/public/glic_api_metrics.cc
- M chrome/browser/glic/public/glic_api_metrics.h
- M chrome/browser/resources/glic/glic_api/glic_api.ts
- M chrome/browser/resources/glic/glic_api/glic_api_generated.ts
- M chrome/browser/resources/glic/glic_api_impl/client/glic_api_client.ts
- M chrome/browser/resources/glic/glic_api_impl/conversions.ts
- M chrome/test/data/webui/glic/browser_tests/glic_api_browsertest.ts
- M tools/metrics/histograms/metadata/glic/enums.xml
- M tools/metrics/histograms/metadata/glic/histograms.xml
Code-Review: +1 by Bryant Chandler, +1 by Trevor Perrier, +1 by Matthew Denton
Review-Enforcement: Code-Review+1 by Bryant Chandler, Code-Review+1 by Trevor Perrier, Code-Review+1 by Matthew Denton
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |