ICU-13842 Use GetDynamicTimeZoneInformation instead of Windows registry access in wintz.cpp #55

mingyzha · 2018-08-13T19:24:29Z

Use GetDynamicTimeZoneInformation instead of accessing the Windows registries so that it does not rely on COM in wintz. So that UWP apps could also use this code.

Checklist

Issue filed at https://unicode-org.atlassian.net : ICU-13842
Update PR title to include Issue number
Issue accepted
Tests included
Documentation is changed or added

CLAassistant · 2018-08-13T19:24:36Z

All committers have signed the CLA.

axelandrejs · 2018-08-13T19:38:11Z

icu4c/source/common/wintz.cpp

-// This file contains only desktop Windows behavior
-// Windows UWP calls Windows::Globalization directly, so this isn't needed there.
-#if U_PLATFORM_USES_ONLY_WIN32_API && (U_PLATFORM_HAS_WINUWP_API == 0)
+#if U_PLATFORM_USES_ONLY_WIN32_API || U_PLATFORM_HAS_WINUWP_API


f U_PLATFORM_USES_ONLY_WIN32_API || U_PLATFORM_HAS_WINUWP_API [](start = 2, length = 61)

Doesn't this also require changes elsewhere, specifically in the caller? putil.cpp has this, which seems like it is wrong now:

#if U_PLATFORM_HAS_WINUWP_API == 0

include "wintz.h"

#else // U_PLATFORM_HAS_WINUWP_API
typedef PVOID LPMSG; // TODO: figure out how to get rid of this typedef
#include <Windows.Globalization.h>
#include <windows.system.userprofile.h>
#include <wrl/wrappers/corewrappers.h>
#include <wrl/client.h>

using namespace ABI::Windows::Foundation;
using namespace Microsoft::WRL;
using namespace Microsoft::WRL::Wrappers;
#endif

And then of course the actual call site. #Closed

The new commits fixed the related codes for this change.

jefgen · 2018-08-13T19:32:56Z

icu4c/source/common/wintz.cpp


  Author: Alan Liu
  Since: ICU 2.6
  Based on original code by Carl Brown <cbrown@xnetinc.com>
+
+  Modified on 8/13/2018


Please remove this comment.
I don't think any changes/modification history are recorded in the source comments anymore. (Or I haven't see anyone do this recently). I would suggest removing this comment, as the change history is stored in Git now.

jefgen · 2018-08-13T19:36:50Z

icu4c/source/common/wintz.cpp

        icuid = (char*)uprv_calloc(len + 1, sizeof(char));
        if (icuid != NULL) 
        {
-            uprv_strcpy(icuid, tmpid);
+            uprv_wcstombs(icuid, dynamicTZI.TimeZoneKeyName, len + 1);


This isn't quite right. This will actually end up using wcstombs to convert the string, meaning that it will be converted from UTF-16 (wide string) to a code-page dependent string -- which isn't what you want at all.

jefgen · 2018-08-13T19:37:13Z

icu4c/source/common/wintz.cpp

@@ -36,174 +34,16 @@

 #define MAX_LENGTH_ID 40


Is this used anymore?

The latest update uses it.

jefgen · 2018-08-13T19:40:29Z

icu4c/source/common/wintz.cpp

@@ -13,14 +13,12 @@

 #include "unicode/utypes.h"

-// This file contains only desktop Windows behavior


This file is currently excluded from the UWP project file for the common project. If the plan is to replace the Windows.Globalization code with this API call (which is goodness), then you will need to modify the common_uwp.vcxproj file too.

The file is updated in the latest change.

jefgen · 2018-08-13T19:42:27Z

icu4c/source/common/wintz.cpp

    {
-        UResourceBundle* winTZ = ures_getByKey(bundle, regStdName, NULL, &status);


I think you still need to look up the ICU time zone in the resources. You can't just take the Windows time zone string from the API and just return that.
You need to handle the Windows -> IANA/CLDR time zone mapping.

jefgen · 2018-08-13T19:45:26Z

icu4c/source/common/wintz.cpp

        }
    }
-
-    ures_close(bundle);

    return icuid;


Where is the mapping between the Windows Time zone names and the ICU Time zone names handled? I don't see it anywhere in here. In fact it looks like you deleted the code that currently does that. (Perhaps that was an accident?)

axelandrejs

🕐

revoking review

jefgen · 2018-08-21T00:06:35Z

icu4c/source/common/wintz.h

@@ -18,7 +18,7 @@

 // This file contains only desktop windows behavior
 // Windows UWP calls Windows::Globalization directly, so this isn't needed there.


This comment is no longer correct. You should likely remove it. #Resolved

jefgen · 2018-08-21T00:10:28Z

icu4c/source/common/wintz.h

@@ -18,7 +18,7 @@

 // This file contains only desktop windows behavior
 // Windows UWP calls Windows::Globalization directly, so this isn't needed there.
-#if U_PLATFORM_USES_ONLY_WIN32_API && (U_PLATFORM_HAS_WINUWP_API == 0)
+#if U_PLATFORM_USES_ONLY_WIN32_API || U_PLATFORM_HAS_WINUWP_API



I think this could probably be changed to just U_PLATFORM_HAS_WIN32_API .

jefgen · 2018-08-21T00:11:42Z

icu4c/source/common/wintz.cpp

-// This file contains only desktop Windows behavior
-// Windows UWP calls Windows::Globalization directly, so this isn't needed there.
-#if U_PLATFORM_USES_ONLY_WIN32_API && (U_PLATFORM_HAS_WINUWP_API == 0)
+#if U_PLATFORM_USES_ONLY_WIN32_API || U_PLATFORM_HAS_WINUWP_API



This could likely be changed to just U_PLATFORM_HAS_WIN32_API.

jefgen · 2018-08-21T00:19:05Z

icu4c/source/common/wintz.cpp

-    if(ERROR_SUCCESS == result) 
+    /* Convert the wchar_t* standard name to char* */
+    uprv_memset(dynamicTZKeyName, 0, sizeof(dynamicTZKeyName));
+    wcstombs(dynamicTZKeyName, dynamicTZI.TimeZoneKeyName, MAX_LENGTH_ID);


wcstombs [](start = 4, length = 8)

We still don't want to use/call this API. The results that it gives are OS-locale/codepage dependent, and the API that we are calling now is not.
GetDynamicTimeZoneInformation TimeZoneKeyName is UTF-16 encoded.

We should probably use something like u_strToUTF8 instead.

See the PR here for an example of how to use it:
https://github.com/unicode-org/icu/pull/31/files

In reply to: 211446320 [](ancestors = 211446320)

jefgen · 2018-08-21T00:20:49Z

icu4c/source/common/wintz.cpp

@@ -36,219 +34,31 @@

 #define MAX_LENGTH_ID 40


MAX_LENGTH_ID [](start = 8, length = 13)

Note: The size/number is not correct anymore.

TimeZoneKeyName can be up to 128 WCHARs long, so 40 is way to small now. :)

Fixed. Thanks

Fixed. Thanks!

jefgen · 2018-08-21T00:22:40Z

icu4c/source/common/wintz.cpp

 {
    UErrorCode status = U_ZERO_ERROR;
    UResourceBundle* bundle = NULL;
    char* icuid = NULL;
-    char apiStdName[MAX_LENGTH_ID];
-    char regStdName[MAX_LENGTH_ID];
+    char dynamicTZKeyName[MAX_LENGTH_ID];


Can we zero init this like this?:
char dynamicTZKeyName[MAX_LENGTH_ID] = {};

Fixed. Thanks!

jefgen

Thanks for the update @mingyzha !
It is looking better, but please take a look at the recent comments. Thanks!

jefgen · 2018-08-22T18:59:21Z

icu4c/source/common/wintz.cpp

-    SYSTEMTIME standardDate;
-    SYSTEMTIME daylightDate;
-} TZI;
+#define MAX_LENGTH_ID 128


Maybe add a comment for this define to indicate where the value comes from?
Nit: Also I wonder if MAX_TIMEZONE_ID_LENGTH might be slightly more clear? (Up to you)

Comments added. Thanks!

axelandrejs · 2018-08-23T00:00:59Z

icu4c/source/common/wintz.cpp

-    wcstombs(apiStdName, apiTZI.StandardName, MAX_LENGTH_ID);
+    /* Obtain TIME_ZONE_INFORMATION from the API and get the non-localized time zone name. */
+    uprv_memset(&dynamicTZI, 0, sizeof(dynamicTZI));
+    GetDynamicTimeZoneInformation(&dynamicTZI);


GetDynamicTimeZoneInformation [](start = 4, length = 29)

Check for failure? #Closed

jefgen · 2018-08-23T19:32:23Z

Thanks @mingyzha!
Note: Please make sure to use the "Squash and merge" option, and also to manually edit the Title before merging to ensure that it has the ICU ticket number.

axelandrejs

…ry access in wintz.cpp (unicode-org#55) Use GetDynamicTimeZoneInformation instead of accessing the Windows registries so that it does not rely on COM in wintz. So that UWP apps could also use this code.

This reverts commit ccad447 in M71 branch. There's an issue with DST detection so that we revert it in M71 branch. For M72 or later, we'll find a better fix. > Fix the timezone detection on Windows with RDP. > > This is to cherry-pick fixes from the upstream to fix > the timezone detection issue when a remote Windows > session is used. The detected timezone should be that of > a remote machine Chrome is running on, but without this > fix, it uses the timezone of machine where a RDP connection > originates. > > Bugs: > > https://unicode-org.atlassian.net/browse/ICU-13842 > https://unicode-org.atlassian.net/browse/ICU-13827 > > Fixes (included in the upstream ICU 63 release candidate) > unicode-org/icu#55 > unicode-org/icu#129 > > TBR=yangguo@chromium.org > Bug: 854387 > Test: Manual. See the bug > Change-Id: Ic21e82987c1569e107d9b5e17bdc0d21e65fda3c > Reviewed-on: https://chromium-review.googlesource.com/c/1270635 Bug: 854387, 913298

…h RDP."" This reverts commit 751b643, which reverted the following earlier commit (ccad447) to deal with a regression on Windows 7 timezone detection. (https://crbug.com/913298) This CL will reintroduce the change made in ccad447 for bug 854387 to M71 branch and be followed by the proper fix for bug 913298. > Fix the timezone detection on Windows with RDP. > > This is to cherry-pick fixes from the upstream to fix > the timezone detection issue when a remote Windows > session is used. The detected timezone should be that of > a remote machine Chrome is running on, but without this > fix, it uses the timezone of machine where a RDP connection > originates. > > Bugs: > > https://unicode-org.atlassian.net/browse/ICU-13842 > https://unicode-org.atlassian.net/browse/ICU-13827 > > Fixes (included in the upstream ICU 63 release candidate) > unicode-org/icu#55 > unicode-org/icu#129 > > TBR=yangguo@chromium.org > Bug: 854387 > Test: Manual. See the bug > Change-Id: Ic21e82987c1569e107d9b5e17bdc0d21e65fda3c > Reviewed-on: https://chromium-review.googlesource.com/c/1270635 Bug: 854387, 913298 Test: Manual. See the bug

…verter add comment about ratesInfo param in UnitConverter

mingyzha added 2 commits August 9, 2018 12:21

clean up wintz.cpp

31ddc77

Edit comments

3c2a968

mingyzha assigned jefgen Aug 13, 2018

mingyzha requested review from ShawnSteele, jefgen, axelandrejs and daniel-ju August 13, 2018 19:24

axelandrejs reviewed Aug 13, 2018

View reviewed changes

jefgen requested changes Aug 13, 2018

View reviewed changes

jefgen reviewed Aug 13, 2018

View reviewed changes

axelandrejs previously requested changes Aug 13, 2018

View reviewed changes

mingyzha added 2 commits August 20, 2018 15:36

Some fixes

f7265dc

edit comments

eba9e56

jefgen reviewed Aug 21, 2018

View reviewed changes

jefgen requested changes Aug 21, 2018

View reviewed changes

Fix PR comments

971dadf

jefgen reviewed Aug 22, 2018

View reviewed changes

Add comments

96f5ecd

axelandrejs reviewed Aug 23, 2018

View reviewed changes

Some Fixes

b68eed2

jefgen approved these changes Aug 23, 2018

View reviewed changes

axelandrejs approved these changes Aug 23, 2018

View reviewed changes

mingyzha merged commit a3f8fc6 into unicode-org:master Aug 23, 2018

hugovdm pushed a commit to hugovdm/icu that referenced this pull request Aug 17, 2020

Merge pull request unicode-org#55 from icu-units/fix_comment_unit_con…

cbed636

…verter add comment about ratesInfo param in UnitConverter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-13842 Use GetDynamicTimeZoneInformation instead of Windows registry access in wintz.cpp #55

ICU-13842 Use GetDynamicTimeZoneInformation instead of Windows registry access in wintz.cpp #55

mingyzha commented Aug 13, 2018 •

edited by jefgen

Loading

CLAassistant commented Aug 13, 2018 •

edited

Loading

axelandrejs Aug 13, 2018 •

edited

Loading

mingyzha Aug 20, 2018

jefgen Aug 13, 2018 •

edited

Loading

jefgen Aug 13, 2018

jefgen Aug 13, 2018

mingyzha Aug 20, 2018

jefgen Aug 13, 2018

mingyzha Aug 20, 2018

jefgen Aug 13, 2018

jefgen Aug 13, 2018

axelandrejs left a comment

jefgen Aug 21, 2018 •

edited

Loading

jefgen Aug 21, 2018

jefgen Aug 21, 2018

jefgen Aug 21, 2018

jefgen Aug 21, 2018

jefgen Aug 21, 2018

mingyzha Aug 22, 2018

mingyzha Aug 22, 2018

jefgen Aug 21, 2018

mingyzha Aug 22, 2018

jefgen left a comment

jefgen Aug 22, 2018

mingyzha Aug 22, 2018

axelandrejs Aug 23, 2018 •

edited

Loading

jefgen commented Aug 23, 2018

axelandrejs left a comment

		@@ -13,14 +13,12 @@

		#include "unicode/utypes.h"

		// This file contains only desktop Windows behavior

		{
		UResourceBundle* winTZ = ures_getByKey(bundle, regStdName, NULL, &status);

		@@ -18,7 +18,7 @@

		// This file contains only desktop windows behavior
		// Windows UWP calls Windows::Globalization directly, so this isn't needed there.

ICU-13842 Use GetDynamicTimeZoneInformation instead of Windows registry access in wintz.cpp #55

ICU-13842 Use GetDynamicTimeZoneInformation instead of Windows registry access in wintz.cpp #55

Conversation

mingyzha commented Aug 13, 2018 • edited by jefgen Loading

Checklist

CLAassistant commented Aug 13, 2018 • edited Loading

axelandrejs Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

include "wintz.h"

Choose a reason for hiding this comment

jefgen Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelandrejs left a comment

Choose a reason for hiding this comment

jefgen Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axelandrejs Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

jefgen commented Aug 23, 2018

axelandrejs left a comment

Choose a reason for hiding this comment

mingyzha commented Aug 13, 2018 •

edited by jefgen

Loading

CLAassistant commented Aug 13, 2018 •

edited

Loading

axelandrejs Aug 13, 2018 •

edited

Loading

jefgen Aug 13, 2018 •

edited

Loading

jefgen Aug 21, 2018 •

edited

Loading

axelandrejs Aug 23, 2018 •

edited

Loading