timezone match code - consider canonical id as well

Improve the timezone matching

GetKnownTimezoneOrNull() is supposed to find a matching timezone entry
to an input tz id in the CrOS tz list.  When looking for a match, the
canonical ids have to be taken into account.

Note: This CL started as a speculative fix to bug 466014, but
I couldn't verify that it fixes the bug on CrOS. It does fix the bug
when I tested 'Chrome with cros=1' on Linux (i.e. stub implementation.)
with tz changed on Linux.

BUG=466014
TEST=manual: see the bug

Committed: https://crrev.com/77e507bdf737ccbf193352ebd09554fea12c2f44
Cr-Commit-Position: refs/heads/master@{#326709}

[jungshik at Google, 2015-04-16 00:30:08]

jshin@chromium.org changed reviewers:
+ pneubeck@chromium.org

[jungshik at Google, 2015-04-16 00:30:22]

PTAL, thanks.

[pneubeck (no reviews), 2015-04-16 08:48:27]

please consider adding a regression test for this.
lgtm (i only checked the documentation of getCanonicalID, so I rely on your
testing that this is actually working)

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
File chromeos/settings/timezone_settings.cc (right):

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
chromeos/settings/timezone_settings.cc:379: status = U_ZERO_ERROR;
not required

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
chromeos/settings/timezone_settings.cc:390: for
(std::vector<icu::TimeZone*>::const_iterator iter = timezones_.begin();
please use the new C++11 range based loops (you can change the old for-loop for
consistency)

[jungshik at Google, 2015-04-17 11:35:05]

On 2015/04/16 08:48:27, pneubeck wrote:
> please consider adding a regression test for this.
> lgtm (i only checked the documentation of getCanonicalID, so I rely on your
> testing that this is actually working)

Thanks. Added a unittest for GetKnown..(). Would you take another look if you
like to?

[jungshik at Google, 2015-04-17 11:35:29]

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
File chromeos/settings/timezone_settings.cc (right):

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
chromeos/settings/timezone_settings.cc:379: status = U_ZERO_ERROR;
On 2015/04/16 08:48:27, pneubeck wrote:
> not required

Although it should not happen, a previous failure (setting it to a failure
value) would lead the current call to bail out immediately (doing nothing). I
added DCHECK() because the failure should not happen.

https://codereview.chromium.org/1082323003/diff/40001/chromeos/settings/timez...
chromeos/settings/timezone_settings.cc:390: for
(std::vector<icu::TimeZone*>::const_iterator iter = timezones_.begin();
On 2015/04/16 08:48:27, pneubeck wrote:
> please use the new C++11 range based loops (you can change the old for-loop
for
> consistency)

Done.

[jungshik at Google, 2015-04-17 11:36:17]

jshin@chromium.org changed reviewers:
+ oshima@chromium.org

[jungshik at Google, 2015-04-17 11:36:18]

oshima@chromium.org: Can you take a look?

[pneubeck (no reviews), 2015-04-17 11:37:28]

i think you forgot to upload the unit test file.

[oshima, 2015-04-17 14:56:33]

please upload unittest.

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
File chromeos/settings/timezone_settings_helper.cc (right):

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:22: for (const auto entry :
timezone_list) {
nit: const auto*

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:41: for (const auto entry :
timezone_list) {
ditto

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:43: known_timezone = entry;
This uses the last timezone that has the same rule. Is it intentional, or should
this return here?

[jungshik at Google, 2015-04-21 13:14:30]

Updated ,but forgot to reply. Thanks for taking a look.

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
File chromeos/settings/timezone_settings_helper.cc (right):

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:22: for (const auto entry :
timezone_list) {
On 2015/04/17 14:56:33, oshima wrote:
> nit: const auto*

Done.

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:41: for (const auto entry :
timezone_list) {
On 2015/04/17 14:56:33, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1082323003/diff/80001/chromeos/settings/timez...
chromeos/settings/timezone_settings_helper.cc:43: known_timezone = entry;
On 2015/04/17 14:56:33, oshima wrote:
> This uses the last timezone that has the same rule. Is it intentional, or
should
> this return here?

Thank you for pointing this out, which made me realize that my original
diagnosis of the bug (that I tried to fix with this CL) was incorrect because I
misread the origiinal code. This CL ended up fixing a situation when running a
CrOS-build of Chrome on Linux (stub implementation), but not on devices. It's
the result of multiple coincidence (on Linux, 1. Canada/Pacific is picked up by
ICU 2. America/Vancouver and America/Los_Angeles share the rules (seemingly even
historically). 

I folded the second loop into the first one (as did the original code). 

As to why the real CrOS behaves differently (non-stub implementation), I still
have to figure it out.

[jungshik at Google, 2015-04-21 13:14:52]

On 2015/04/17 11:37:28, pneubeck wrote:
> i think you forgot to upload the unit test file.

Oops. Sorry. I uploaded a new CL.

[oshima, 2015-04-23 18:30:45]

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
File chromeos/settings/timezone_settings_unittest.cc (right):

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:12: namespace {
move this inside chromeos::system

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:14: const static char*
kTimeZones[] = {
static isn't necessary in anonymous namespace

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:41: for (auto id : kTimeZones) {
const auto*

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:73: {
any reason for this extra block?

[jungshik at Google, 2015-04-23 19:48:12]

thanks for taking a look. Can you take another look?

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
File chromeos/settings/timezone_settings_unittest.cc (right):

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:12: namespace {
On 2015/04/23 18:30:45, oshima wrote:
> move this inside chromeos::system

Done.

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:14: const static char*
kTimeZones[] = {
On 2015/04/23 18:30:45, oshima wrote:
> static isn't necessary in anonymous namespace

Acknowledged.

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:41: for (auto id : kTimeZones) {
On 2015/04/23 18:30:45, oshima wrote:
> const auto*

Done. Do you prefer 'const char*' or 'const auto*' here? Down below, I used
'const char*'. I changed that to 'const auto*' to be more consistent.

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:73: {
On 2015/04/23 18:30:45, oshima wrote:
> any reason for this extra block?

oops. what was I thinking? It's not necessary.

[oshima, 2015-04-23 22:01:24]

please remove "static", then lgtm.

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
File chromeos/settings/timezone_settings_unittest.cc (right):

https://codereview.chromium.org/1082323003/diff/140001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:41: for (auto id : kTimeZones) {
On 2015/04/23 19:48:12, Jungshik at google wrote:
> On 2015/04/23 18:30:45, oshima wrote:
> > const auto*
> 
> Done. Do you prefer 'const char*' or 'const auto*' here? Down below, I used
> 'const char*'. I changed that to 'const auto*' to be more consistent. 

I'd use auto only if the type is long. More explicit is better, although I'm
fine with auto as well as this is obvious case.

https://codereview.chromium.org/1082323003/diff/160001/chromeos/settings/time...
File chromeos/settings/timezone_settings_unittest.cc (right):

https://codereview.chromium.org/1082323003/diff/160001/chromeos/settings/time...
chromeos/settings/timezone_settings_unittest.cc:18: const static char*
kTimeZones[] = {
i meant "please remove static" as this is redundant.
in c++11, this has internal linkage.

[jungshik at Google, 2015-04-23 22:11:46]

Thanks. Review comments addressed.

[jungshik at Google, 2015-04-23 22:11:54]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-04-23 22:11:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1082323003/#ps180001
(title: "static removed")

[commit-bot: I haz the power, 2015-04-23 22:12:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082323003/180001

[commit-bot: I haz the power, 2015-04-23 22:24:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 22:24:13]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jungshik at Google, 2015-04-23 23:30:07]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2015-04-23 23:31:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082323003/180001

[commit-bot: I haz the power, 2015-04-23 23:41:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-23 23:41:53]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jungshik at Google, 2015-04-24 00:23:14]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-04-24 00:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1082323003/#ps200001
(title: "update gn build")

[commit-bot: I haz the power, 2015-04-24 00:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082323003/200001

[commit-bot: I haz the power, 2015-04-24 01:13:24]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-24 01:14:09]

Patchset 11 (id:??) landed as
https://crrev.com/77e507bdf737ccbf193352ebd09554fea12c2f44
Cr-Commit-Position: refs/heads/master@{#326709}