Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU-13827 Clean up ICU4C "wintz.cpp" time zone detection code. #129

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

jefgen
Copy link
Member

@jefgen jefgen commented Sep 17, 2018

  • Use stack allocated UResouceBundle to reduce number of calls to malloc (in a method that can't report back an error if Out-Of-Memory happens).
  • Use LocalUResourcePointer for automatic clean-up of UResouceBundle.
  • Use uprv_strdup instead of calloc + strcpy.
  • Changes comments, formatting, etc.
Checklist

@jefgen jefgen changed the title ICU-13827 Clean up wintz.cpp. ICU-13827 Clean up ICU4C "wintz.cpp" time zone detection code. Sep 17, 2018
}

ures_close(bundle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to close the new bundle pointer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the neat thing about LocalUResourceBundlePointer, it will automatically call ures_close when the object goes out of scope. So we don't have to worry about remembering to call close. :)

Copy link
Member

@yumaoka yumaoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for cleaning up

@jefgen jefgen merged commit 9385223 into unicode-org:master Sep 19, 2018
@jefgen jefgen deleted the jefgen/wintz branch September 19, 2018 07:49
sharing-sense pushed a commit to sharing-sense/chromium-deps-icu that referenced this pull request Mar 13, 2019
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
sharing-sense pushed a commit to sharing-sense/chromium-deps-icu that referenced this pull request Mar 13, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants