Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2061633002:
Separate time variables for 32 and 64 bit systems in policy_browsertest (Closed)
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate time variables for 32 and 64 bit systems in policy_browsertest
Some test cases fail due to updates to the time conversion
functions - https://codereview.chromium.org/1988663002/
The problem is that linux_chromium_dbg_32 starts to fail,
when the above mentioned CL is applied, because the date
after 2038 cannot be properly converted on 32-bit systems
due to 2^32 value limitation.
Committed: https://crrev.com/18b63eadc7c74c356adb148cdd863cc587ec2bdd
Cr-Commit-Position: refs/heads/master@{#399746}
Patch Set 1 #Patch Set 2 : TODO added #
Total comments: 1
Patch Set 3 : use year 2038 for everything #Messages
Total messages: 21 (6 generated)
Description was changed from ========== 32bit BUG= ========== to ========== Separate time variables for 32 and 64 bit systems. Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ==========
maksim.sisov@intel.com changed reviewers: + bartfab@chromium.org, pastarmovj@chromium.org
Description was changed from ========== Separate time variables for 32 and 64 bit systems. Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ========== to ========== Separate time variables for 32 and 64 bit systems in policy_browsertest Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ==========
please review
Just a quick question: How bad is it actually use only the 2038 year for this test. I think being set for the next 20 years (assuming this date has to be in the future) should be good enough, right? This test is not focused on whether we can parse the date always correctly in all cases (which is covered by the Time unit tests) but whether we act well on the cookie being set? Presumably by 2038 the 32 bit version will be gone and we will be able to fix this date then :)
On 2016/06/13 10:22:57, pastarmovj wrote: > Just a quick question: How bad is it actually use only the 2038 year for this > test. I think being set for the next 20 years (assuming this date has to be in > the future) should be good enough, right? This test is not focused on whether we > can parse the date always correctly in all cases (which is covered by the Time > unit tests) but whether we act well on the cookie being set? Presumably by 2038 > the 32 bit version will be gone and we will be able to fix this date then :) Yes, time conversion failures are tested in time unittests and these tests are not responsible for that. I think event the 32 bit version of Chromium is almost gone. As far as I am concerned, there is no support for ubuntu 12 32-bit anymore.
On 2016/06/13 10:22:57, pastarmovj wrote: > Just a quick question: How bad is it actually use only the 2038 year for this > test. I think being set for the next 20 years (assuming this date has to be in > the future) should be good enough, right? Yes, cookies do not live that long. I guess it should be allright, but we can double check. jww@, would you please answer?
On 2016/06/13 10:44:38, maksims wrote: > On 2016/06/13 10:22:57, pastarmovj wrote: > > Just a quick question: How bad is it actually use only the 2038 year for this > > test. I think being set for the next 20 years (assuming this date has to be in > > the future) should be good enough, right? > > Yes, cookies do not live that long. I guess it should be allright, but we can > double > check. > > jww@, would you please answer? Yes, this sounds fine to me, although I'd love to have a TODO above it and a link to a specific bug about getting rid of this eventually. Would one of you mind filing said bug?
On 2016/06/13 17:12:12, jww wrote: > On 2016/06/13 10:44:38, maksims wrote: > > On 2016/06/13 10:22:57, pastarmovj wrote: > > > Just a quick question: How bad is it actually use only the 2038 year for > this > > > test. I think being set for the next 20 years (assuming this date has to be > in > > > the future) should be good enough, right? > > > > Yes, cookies do not live that long. I guess it should be allright, but we can > > double > > check. > > > > jww@, would you please answer? > > Yes, this sounds fine to me, although I'd love to have a TODO above it and a > link to a specific bug about getting rid of this eventually. Would one of you > mind filing said bug? Yes, sure. But it needs to be modified a bit (remove automatically added lines). I've already asked to do so as long as I don't have such rights https://crbug.com/619828
please review
reminder
https://codereview.chromium.org/2061633002/diff/20001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2061633002/diff/20001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:250: // in the future. https://crbug.com/619828 What I meant however was to remove the #ifdef completely in favor of the 32bit test case for now. Then the todo would be to change this to the year 3000 once no more 32 bit version is ever built.
Well, we can do it like this as well. Btw, what was the reason you used year 3000 here?
On 2016/06/14 17:21:55, maksims wrote: > Well, we can do it like this as well. > > Btw, what was the reason you used year 3000 here? I guess random choice far enough in the future. Someone wanted to be safe for the next thousand years. :)
lgtm
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2061633002/40001
Message was sent while issue was closed.
Description was changed from ========== Separate time variables for 32 and 64 bit systems in policy_browsertest Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ========== to ========== Separate time variables for 32 and 64 bit systems in policy_browsertest Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Separate time variables for 32 and 64 bit systems in policy_browsertest Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. ========== to ========== Separate time variables for 32 and 64 bit systems in policy_browsertest Some test cases fail due to updates to the time conversion functions - https://codereview.chromium.org/1988663002/ The problem is that linux_chromium_dbg_32 starts to fail, when the above mentioned CL is applied, because the date after 2038 cannot be properly converted on 32-bit systems due to 2^32 value limitation. Committed: https://crrev.com/18b63eadc7c74c356adb148cdd863cc587ec2bdd Cr-Commit-Position: refs/heads/master@{#399746} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/18b63eadc7c74c356adb148cdd863cc587ec2bdd Cr-Commit-Position: refs/heads/master@{#399746} |
