|
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 1 month ago CC:
chromium-reviews, fukino Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake callers of FromUTC(Local)Exploded in google_apis/ use new time API.
Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/
BUG=601900
Committed: https://crrev.com/e20a6fdd7a74a53b83a8be5a1db0119e77f8cacc
Cr-Commit-Position: refs/heads/master@{#430660}
Patch Set 1 #
Total comments: 8
Patch Set 2 : rebased #Patch Set 3 : rebased #Patch Set 4 : rebased #Patch Set 5 : rebased #Patch Set 6 : typo #Patch Set 7 : rebased and set dependent patch #
Total comments: 2
Patch Set 8 : comments #
Depends on Patchset: Messages
Total messages: 55 (38 generated)
Description was changed from ========== google apis time callers BUG= ========== to ========== Make callers of FromUTC(Local)Exploded in google_apis/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601900 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
please review
maksim.sisov@intel.com changed reviewers: + hidehiko@chromium.org
please review
FYI: fukino@. Could you run trybots? https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_requests_unittest.cc (right): https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:569: base::Time LastViewedByMeDateUTC; base::Time last_viewed_by_me_date_utc; to follow the style guide. Ditto for below. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:570: EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, Could you use ASSERT_TRUE? Ditto for below. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:572: nit: could you remove this empty line? L573 looks to belong to L569-571 block conceptually, to me. Ditto for below.
The CQ bit was checked by fukino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by fukino@chromium.org
On 2016/07/05 17:31:14, hidehiko wrote: > FYI: fukino@. > > Could you run trybots? > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > File google_apis/drive/drive_api_requests_unittest.cc (right): > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > google_apis/drive/drive_api_requests_unittest.cc:569: base::Time > LastViewedByMeDateUTC; > base::Time last_viewed_by_me_date_utc; > > to follow the style guide. Ditto for below. > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > google_apis/drive/drive_api_requests_unittest.cc:570: > EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, > Could you use ASSERT_TRUE? Ditto for below. > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > google_apis/drive/drive_api_requests_unittest.cc:572: > nit: could you remove this empty line? L573 looks to belong to L569-571 block > conceptually, to me. Ditto for below. I run other tests here - https://codereview.chromium.org/2128483003/ https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... Seems like 1 millisecond is lost after time is converted in mac implementation. Any guesses why it happens?
On 2016/07/06 12:18:40, maksims wrote: > On 2016/07/05 17:31:14, hidehiko wrote: > > FYI: fukino@. > > > > Could you run trybots? > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > File google_apis/drive/drive_api_requests_unittest.cc (right): > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > google_apis/drive/drive_api_requests_unittest.cc:569: base::Time > > LastViewedByMeDateUTC; > > base::Time last_viewed_by_me_date_utc; > > > > to follow the style guide. Ditto for below. > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > google_apis/drive/drive_api_requests_unittest.cc:570: > > EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, > > Could you use ASSERT_TRUE? Ditto for below. > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > google_apis/drive/drive_api_requests_unittest.cc:572: > > nit: could you remove this empty line? L573 looks to belong to L569-571 block > > conceptually, to me. Ditto for below. > > I run other tests here - https://codereview.chromium.org/2128483003/ > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... > > Seems like 1 millisecond is lost after time is converted in mac implementation. > Any guesses why it happens? Looks like a rounding error? # I do not have Mac env, so this is just my guess, though... |(2005/01/07 08:02.123) - (1970/01/01 00:00)| ~ 1105084920.123 secs. Internally, it is converted into millisecond unit, then casted to int64. https://cs.chromium.org/chromium/src/base/time/time_mac.cc?q=kWindowsEpochDel... 1105084920.123 * 1000000 = 1105084920122999.9 in 64bit fp math. So, if usec part is truncated, 1 millisec error can happen?
On 2016/07/07 17:23:33, hidehiko wrote: > On 2016/07/06 12:18:40, maksims wrote: > > On 2016/07/05 17:31:14, hidehiko wrote: > > > FYI: fukino@. > > > > > > Could you run trybots? > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > File google_apis/drive/drive_api_requests_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > google_apis/drive/drive_api_requests_unittest.cc:569: base::Time > > > LastViewedByMeDateUTC; > > > base::Time last_viewed_by_me_date_utc; > > > > > > to follow the style guide. Ditto for below. > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > google_apis/drive/drive_api_requests_unittest.cc:570: > > > EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, > > > Could you use ASSERT_TRUE? Ditto for below. > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > google_apis/drive/drive_api_requests_unittest.cc:572: > > > nit: could you remove this empty line? L573 looks to belong to L569-571 > block > > > conceptually, to me. Ditto for below. > > > > I run other tests here - https://codereview.chromium.org/2128483003/ > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... > > > > Seems like 1 millisecond is lost after time is converted in mac > implementation. > > Any guesses why it happens? > > Looks like a rounding error? > # I do not have Mac env, so this is just my guess, though... > > |(2005/01/07 08:02.123) - (1970/01/01 00:00)| ~ 1105084920.123 secs. > > Internally, it is converted into millisecond unit, then casted to int64. > https://cs.chromium.org/chromium/src/base/time/time_mac.cc?q=kWindowsEpochDel... > > 1105084920.123 * 1000000 = 1105084920122999.9 in 64bit fp math. > So, if usec part is truncated, 1 millisec error can happen? Me neither, I have linux only, but I have mac at home. Will check that during the weekend.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
The CQ bit was unchecked by maksim.sisov@intel.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/08 12:25:10, maksims wrote: > On 2016/07/07 17:23:33, hidehiko wrote: > > On 2016/07/06 12:18:40, maksims wrote: > > > On 2016/07/05 17:31:14, hidehiko wrote: > > > > FYI: fukino@. > > > > > > > > Could you run trybots? > > > > > > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > > File google_apis/drive/drive_api_requests_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > > google_apis/drive/drive_api_requests_unittest.cc:569: base::Time > > > > LastViewedByMeDateUTC; > > > > base::Time last_viewed_by_me_date_utc; > > > > > > > > to follow the style guide. Ditto for below. > > > > > > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > > google_apis/drive/drive_api_requests_unittest.cc:570: > > > > EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, > > > > Could you use ASSERT_TRUE? Ditto for below. > > > > > > > > > > > > > > https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... > > > > google_apis/drive/drive_api_requests_unittest.cc:572: > > > > nit: could you remove this empty line? L573 looks to belong to L569-571 > > block > > > > conceptually, to me. Ditto for below. > > > > > > I run other tests here - https://codereview.chromium.org/2128483003/ > > > > > > https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel... > > > > > > Seems like 1 millisecond is lost after time is converted in mac > > implementation. > > > Any guesses why it happens? > > > > Looks like a rounding error? > > # I do not have Mac env, so this is just my guess, though... > > > > |(2005/01/07 08:02.123) - (1970/01/01 00:00)| ~ 1105084920.123 secs. > > > > Internally, it is converted into millisecond unit, then casted to int64. > > > https://cs.chromium.org/chromium/src/base/time/time_mac.cc?q=kWindowsEpochDel... > > > > 1105084920.123 * 1000000 = 1105084920122999.9 in 64bit fp math. > > So, if usec part is truncated, 1 millisec error can happen? > > Me neither, I have linux only, but I have mac at home. Will check that during > the weekend. Ok, now it's fixed after I fixed overflows on all the platforms. Please review
Only style comments. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_requests_unittest.cc (right): https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:569: base::Time LastViewedByMeDateUTC; On 2016/07/05 17:31:14, hidehiko wrote: > base::Time last_viewed_by_me_date_utc; > > to follow the style guide. Ditto for below. ping. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:570: EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, On 2016/07/05 17:31:14, hidehiko wrote: > Could you use ASSERT_TRUE? Ditto for below. ping. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:572: On 2016/07/05 17:31:14, hidehiko wrote: > nit: could you remove this empty line? L573 looks to belong to L569-571 block > conceptually, to me. Ditto for below. ping. https://codereview.chromium.org/2091663002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_requests_unittest.cc (right): https://codereview.chromium.org/2091663002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_requests_unittest.cc:644: base::Time::FromUTCExploded(kLastViewedByMeDate); Please fix the indent.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... File google_apis/drive/drive_api_requests_unittest.cc (right): https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:569: base::Time LastViewedByMeDateUTC; On 2016/07/05 17:31:14, hidehiko wrote: > base::Time last_viewed_by_me_date_utc; > > to follow the style guide. Ditto for below. Done. https://codereview.chromium.org/2091663002/diff/80001/google_apis/drive/drive... google_apis/drive/drive_api_requests_unittest.cc:570: EXPECT_TRUE(base::Time::FromUTCExploded(kLastViewedByMeDate, On 2016/07/05 17:31:14, hidehiko wrote: > Could you use ASSERT_TRUE? Ditto for below. Done. https://codereview.chromium.org/2091663002/diff/200001/google_apis/drive/driv... File google_apis/drive/drive_api_requests_unittest.cc (right): https://codereview.chromium.org/2091663002/diff/200001/google_apis/drive/driv... google_apis/drive/drive_api_requests_unittest.cc:644: base::Time::FromUTCExploded(kLastViewedByMeDate); On 2016/10/25 16:37:05, hidehiko wrote: > Please fix the indent. Done.
gentle reminder
On 2016/11/02 06:54:33, maksims wrote: > gentle reminder Oops, very sorry. LGTM. Defer to fukino@.
fukino@, do you have any comments?
LGTM. I'm sorry for a late response!
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make callers of FromUTC(Local)Exploded in google_apis/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601900 ========== to ========== Make callers of FromUTC(Local)Exploded in google_apis/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601900 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Make callers of FromUTC(Local)Exploded in google_apis/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601900 ========== to ========== Make callers of FromUTC(Local)Exploded in google_apis/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601900 Committed: https://crrev.com/e20a6fdd7a74a53b83a8be5a1db0119e77f8cacc Cr-Commit-Position: refs/heads/master@{#430660} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e20a6fdd7a74a53b83a8be5a1db0119e77f8cacc Cr-Commit-Position: refs/heads/master@{#430660} |