IcuBridge migration 10/n: multiple components [chromium/src : main]
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
There's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
Related details
- Danilo Tedeschi
- Maks Orlovich
- Stephen Nusko
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
YMDT::Short()This isn't the same format I think? https://source.chromium.org/chromium/chromium/src/+/main:base/i18n/icubridge/date_time_formatter.h;l=225;drc=78e78403ceca65207ef09360675b0fe86454ee6e
```
// e.g., "5/25/26, 10:30 AM"
static auto Short() {
return Builder<component_type_value, ItemLength::kShort>();
}
```
Which is d-M-Y (inverted) and no seconds (although I see with_time_precision so maybe that adds seconds). Can we not get the exact same format?
Ideally you'd record a trace and ensure it looks the same as well, either linking it or sharing a screenshot.
AddMetadataToBundle(
kTraceCaptureDatetimeKey,
base::UTF16ToUTF8(
IcuBridge::GetInstance().date_time_formatter().Format(
TRACE_TIME_NOW(), GetKnownLanguageTag("en-US"),
YMDT::Short()
.with_time_precision(
base::i18n::DateTimeFormatterOptions::TimePrecision::
kSecond)
.with_time_zone(TimeZone::GMT()))),This is a non-trivial conversion indeed... Why do we have UTF conversion as well? The bug implies this is for a rust conversion, fair enough but feels like perhaps we should have some helpers to clean this up?
Related details
- Danilo Tedeschi
- Maks Orlovich
- Sylvain Defresne
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. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 2 comments![]( "Open in Gerrit")
return base::StringPrintf("%04d-%02d-%02dT%02d:%02d:%02d", exploded.year,This change does not corresponds to the CL description.
The description says that the conversion is done to use the new icu API but instead the time formatting is done using `printf`. Is there no way with the new API to write [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) formatted date time?
Would `base::i18n::datetime_options::YMDT::Short()` work?
return base::UTF16ToUTF8(This is not a 1:1 conversion.
The old code resulted in strings like `"2026-07-29"` while the new code results in strings like `"7/29/26"`.
Related details
- Danilo Tedeschi
- Maks Orlovich
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. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
There's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
+1
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
return base::StringPrintf("%d/%d/%d %d:%d:%d.%d", exploded.year,%02d for all of those at very least, though suggestions for actually using base::i18n made elsewhere are probably better.
Related details
- Danilo Tedeschi
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. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 6 comments![]( "Open in Gerrit")
Sylvain DefresneThere's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
+1
sorry about that, I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files. This is the last change for the migration tho.
return base::StringPrintf("%04d-%02d-%02dT%02d:%02d:%02d", exploded.year,This change does not corresponds to the CL description.
The description says that the conversion is done to use the new icu API but instead the time formatting is done using `printf`. Is there no way with the new API to write [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) formatted date time?
Would `base::i18n::datetime_options::YMDT::Short()` work?
Done
return base::StringPrintf("%d/%d/%d %d:%d:%d.%d", exploded.year,%02d for all of those at very least, though suggestions for actually using base::i18n made elsewhere are probably better.
Done
This is not a 1:1 conversion.
The old code resulted in strings like `"2026-07-29"` while the new code results in strings like `"7/29/26"`.
Done
This isn't the same format I think? https://source.chromium.org/chromium/chromium/src/+/main:base/i18n/icubridge/date_time_formatter.h;l=225;drc=78e78403ceca65207ef09360675b0fe86454ee6e
```
// e.g., "5/25/26, 10:30 AM"
static auto Short() {
return Builder<component_type_value, ItemLength::kShort>();
}
```Which is d-M-Y (inverted) and no seconds (although I see with_time_precision so maybe that adds seconds). Can we not get the exact same format?
Ideally you'd record a trace and ensure it looks the same as well, either linking it or sharing a screenshot.
sorry about that. Done.
AddMetadataToBundle(
kTraceCaptureDatetimeKey,
base::UTF16ToUTF8(
IcuBridge::GetInstance().date_time_formatter().Format(
TRACE_TIME_NOW(), GetKnownLanguageTag("en-US"),
YMDT::Short()
.with_time_precision(
base::i18n::DateTimeFormatterOptions::TimePrecision::
kSecond)
.with_time_zone(TimeZone::GMT()))),This is a non-trivial conversion indeed... Why do we have UTF conversion as well? The bug implies this is for a rust conversion, fair enough but feels like perhaps we should have some helpers to clean this up?
we do have a lot of helpers in base/i18n/time_formatting.h for example there's a one for the ISO standard and a HTTP one, but there is not specific one that you are using here. I will use base::Time Exploded to nail the same pattern.
Related details
- Maks Orlovich
- Mark Mentovai
- Stephen Nusko
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maks Orlovich (Gerrit)
Maks Orlovich added 2 comments![]( "Open in Gerrit")
Sylvain DefresneThere's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
Danilo Tedeschi+1
sorry about that, I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files. This is the last change for the migration tho.
FWIW, git-cl split exists.
Related details
- Danilo Tedeschi
- Mark Mentovai
- Stephen Nusko
- Sylvain Defresne
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. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Sylvain DefresneThere's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
Danilo Tedeschi+1
Maks Orlovichsorry about that, I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files. This is the last change for the migration tho.
FWIW, git-cl split exists.
I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files.
Your agent made a process mistake and you’re just going to let it? Who’s running the show here?
FWIW, git-cl split exists.
Right. Not too late to fix it.
Related details
- Danilo Tedeschi
- Stephen Nusko
- Sylvain Defresne
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 2 comments![]( "Open in Gerrit")
Sylvain DefresneThere's a lot of unrelated stuff in here, and nothing seems to be interdependent. Why are these grouped together? Why not do several reviewer-specific changes instead?
Danilo Tedeschi+1
Maks Orlovichsorry about that, I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files. This is the last change for the migration tho.
Mark MentovaiFWIW, git-cl split exists.
I have an agent that groups targets together for this migration trying to minimize the reviewers but also not have changes with like one or two files.
Your agent made a process mistake and you’re just going to let it? Who’s running the show here?
FWIW, git-cl split exists.
Right. Not too late to fix it.
sorry drop the ball here on this one folks. Was in a hurry to wrap up this migration, no excuse tho.
Fixing it.
Related details
- Maks Orlovich
- Mark Mentovai
- Stephen Nusko
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
1 comment
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +0 |
1 comment
Before landing, please update the CL description.
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 1 comment![]( "Open in Gerrit")
Before landing, please update the CL description.
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
Sorry I didn't mean to remove my +1 with my last comment. Gerrit like to remove +1 when adding comments.
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi 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")
Change information
IcuBridge migration 10/n: multiple components
Migrate deprecated UnlocalizedTimeFormatWithPattern and
LocalizedTimeFormatWithPattern usages to use
base::i18n::IcuBridge::DateTimeFormatter and modern time APIs.
- M ios/chrome/app/dump_documents_statistics.mm
- M ios/chrome/browser/webui/ui_bundled/version_ui.mm
- M rlz/lib/financial_ping_test.cc
Code-Review: +1 by Sylvain Defresne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |