1947875 - Fix wrong use of Some() in AppDateTimeFormat::Format

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Yury]

This is related to bug 1796042 where this observation was made in https://phabricator.services.mozilla.com/D197989#6967031:

timeZoneID doesn't survive Some. The resulting Maybe<Span<const char16_t>> has the right length (3 or 9 characters) but the characters themselves aren't initialised (0xffffffff).

That's correct as the Some() call seems to be wrong. It will be corrected in the patch I'm going to attach. Note that the test fails if the code fix is removed.

Comment 1

[Yury]

Attachment: Bug 1947875 - Fix wrong use of Some() in AppDateTimeFormat::Format. r=gregtatum

Based on https://searchfox.org/mozilla-central/rev/5e7382bf8bbb88d8260c72990cfea9b626d9b307/intl/locale/AppDateTimeFormat.cpp#201,204-205
the usage of Some() needs to be corrected.

Comment 2

[Yury]

Sorry, I mostly work on TB bugs, they have a checkin-needed-tb keyword. How does one request landing this patch?

Comment 3

[Yury]

Looks like adding "Check-in Needed" as a Phab tag is the way to go.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a6f56d7cd12
Fix wrong use of Some() in AppDateTimeFormat::Format. r=gregtatum

Comment 5

[agoloman]

Backed out for causing gtest failures @AppDateTimeFormat.

• Backout link
• Push with failures
• Failure Log

Comment 6

[Yury]

Sorry about the test failure. The test worked locally on Windows. According to the log from Linux, the failure is:
TEST-UNEXPECTED-FAIL | AppDateTimeFormat.FormatPRExplodedTime | Value of: formattedTime.Find(u"-09:13") != kNotFound.

When I get back to my dev machine, I'll see how it behaves on Linux and/or when run in a different time zone.

Comment 7

[Yury]

For the test to pass, it also needs to check for "-9:13" on Linux. I won't have access to my dev machine for a while, so if someone wants to modify the patch like this, that would be appreciated:

+++ b/intl/locale/tests/gtest/TestAppDateTimeFormat.cpp
@@ -106,9 +106,10 @@ TEST(AppDateTimeFormat, FormatPRExploded
   // From above: Wed, 31 Dec 1969 14:47:00 -09:13
   rv = AppDateTimeFormat::Format(components, &prExplodedTime, formattedTime);
   ASSERT_NS_SUCCEEDED(rv);
   ASSERT_TRUE(formattedTime.Find(u"Wed") != kNotFound);
-  ASSERT_TRUE(formattedTime.Find(u"-09:13") != kNotFound);
+  ASSERT_TRUE(formattedTime.Find(u"-09:13") != kNotFound ||
+              formattedTime.Find(u"-9:13") != kNotFound);
 }
 
 TEST(AppDateTimeFormat, DateFormatSelectors)
 {

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04974c6e86d7
Fix wrong use of Some() in AppDateTimeFormat::Format. r=gregtatum

Comment 9

[Yury]

Thanks, welpy-cw, and sorry about my limited availability.

Comment 10

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/04974c6e86d7