Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/416348)
3 years, 8 months ago
(2017-04-20 12:08:03 UTC)
#4
Nit: There's two more occurrences of UnixEpoch in remote_commands_service.cc and in device_status_collector.c. https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/device_management_backend.proto File components/policy/proto/device_management_backend.proto ...
3 years, 8 months ago
(2017-04-20 14:02:13 UTC)
#11
On 2017/04/20 14:02:13, Thiemo Nagel wrote: > Nit: There's two more occurrences of UnixEpoch in ...
3 years, 8 months ago
(2017-04-20 15:00:46 UTC)
#14
On 2017/04/20 14:02:13, Thiemo Nagel wrote:
> Nit: There's two more occurrences of UnixEpoch in remote_commands_service.cc
and
> in device_status_collector.c.
* remote_commands_service.cc: It uses TimeTicks, which, first, technically
provides to conversion to/from Java times, and, second, is weird to me
(see more on this in the comment below).
* device_status_collector.cc: Do you think we should change it too? IIUC,
it has nothing to do with the Java times directly, but is used for
grouping of some data into per-day buckets.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
File components/policy/proto/device_management_backend.proto (right):
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:205: // This is the last
policy timestamp that client received from server.
On 2017/04/20 14:02:13, Thiemo Nagel wrote:
> Nit: Please add "Java time".
Acknowledged, though I decided to refer to PolicyData.timestamp instead. Apart
from ridding of duplication, I think it's useful to hint that the client
shouldn't generate the value for this field itself, using its potentially wrong
clocks.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:289: // [timestamp] is
milliseconds since Epoch in UTC timezone (Java time). It is
On 2017/04/20 14:02:13, Thiemo Nagel wrote:
> Nit: Could you please either re-wrap the whole paragraph at 80 characters or
> stay in line with the existing (70 character) wrapping?
Done.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:561: // Client will send
at most 50 timestamps to DM. All the rest
On 2017/04/20 14:02:13, Thiemo Nagel wrote:
> Nit: Please add "Java time".
I think this message InstallableLaunch is actually unused.
So maybe just mark it all as OBSOLETE_? (in a separate CL)
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:1102: // The time at
which the command was executed, if the the result is
On 2017/04/20 14:02:13, Thiemo Nagel wrote:
> Nit: Please mention "Java time".
I'm afraid this is not applicable in this case. According to the code (see
RemoteCommandsService::OnJobFinished()), this field is set from TimeTicks,
which, IIUC, is weird and has no physical sense. Quoting base/time/time.h:
regarding TimeTicks:
[...] Note that TimeTicks may "stand still" (e.g., if the computer is
suspended) [...]
regarding TimeTicks::UnixEpoch:
Get an estimate of the TimeTick value at the time of the UnixEpoch.
[...] This function will return the same value for the duration of
the application, but will be different in future application runs.
Thiemo Nagel
> * remote_commands_service.cc: It uses TimeTicks, which, first, technically > provides to conversion to/from Java ...
3 years, 8 months ago
(2017-04-20 16:16:53 UTC)
#15
> * remote_commands_service.cc: It uses TimeTicks, which, first, technically
> provides to conversion to/from Java times, and, second, is weird to me
> (see more on this in the comment below).
Agreed, there's nothing to do in remote_commands_service.cc.
> * device_status_collector.cc: Do you think we should change it too? IIUC,
> it has nothing to do with the Java times directly, but is used for
> grouping of some data into per-day buckets.
What they call a DateKey is just the first millisecond of the day in Java time.
Imho it would make sense to use JavaTime conversion here, but entirely up to
you.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
File components/policy/proto/device_management_backend.proto (right):
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:561: // Client will send
at most 50 timestamps to DM. All the rest
> I think this message InstallableLaunch is actually unused.
> So maybe just mark it all as OBSOLETE_? (in a separate CL)
I've added it to my CL deleting unused proto fields.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:1102: // The time at
which the command was executed, if the the result is
> I'm afraid this is not applicable in this case. According to the code (see
> RemoteCommandsService::OnJobFinished()), this field is set from TimeTicks,
> which, IIUC, is weird and has no physical sense.
The way I read this, TimeTicks::UnixEpoch() is a way to approximate the correct
time. In the end, the server expects Java time and the client sends Java time
(albeit only approximated). Thus I'd say it would be correct to annotate this
as Java time, maybe adding a note that it's only an approximation.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago
(2017-04-20 16:35:20 UTC)
#16
On 2017/04/20 16:16:53, Thiemo Nagel wrote: > > * device_status_collector.cc: Do you think we should ...
3 years, 8 months ago
(2017-04-20 18:33:22 UTC)
#20
On 2017/04/20 16:16:53, Thiemo Nagel wrote:
> > * device_status_collector.cc: Do you think we should change it too? IIUC,
> > it has nothing to do with the Java times directly, but is used for
> > grouping of some data into per-day buckets.
>
> What they call a DateKey is just the first millisecond of the day in Java
time.
> Imho it would make sense to use JavaTime conversion here, but entirely up to
> you.
Done. Agreed that as long as the timestamp is transmitted to the server, it's
fine to suppose that it's intentionally in Java time format.
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
File components/policy/proto/device_management_backend.proto (right):
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:561: // Client will send
at most 50 timestamps to DM. All the rest
On 2017/04/20 16:16:52, Thiemo Nagel wrote:
> > I think this message InstallableLaunch is actually unused.
> > So maybe just mark it all as OBSOLETE_? (in a separate CL)
>
> I've added it to my CL deleting unused proto fields.
Thanks!
https://codereview.chromium.org/2830033003/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:1102: // The time at
which the command was executed, if the the result is
On 2017/04/20 16:16:52, Thiemo Nagel wrote:
> > I'm afraid this is not applicable in this case. According to the code (see
> > RemoteCommandsService::OnJobFinished()), this field is set from TimeTicks,
> > which, IIUC, is weird and has no physical sense.
>
> The way I read this, TimeTicks::UnixEpoch() is a way to approximate the
correct
> time. In the end, the server expects Java time and the client sends Java time
> (albeit only approximated). Thus I'd say it would be correct to annotate this
> as Java time, maybe adding a note that it's only an approximation.
OK, I searched through the server code and it looks like this field is actually
interpreted as a Java time. Therefore using approximation is a client bug, while
this proto, as a public contract, should define what this field supposed to
mean.
Done.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago
(2017-04-20 18:37:00 UTC)
#21
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cronet/builds/123774) android_n5x_swarming_rel on ...
3 years, 8 months ago
(2017-04-20 18:37:01 UTC)
#22
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/417435)
3 years, 8 months ago
(2017-04-21 11:19:22 UTC)
#32
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492783382736660, "parent_rev": "402945dae4329cd6e3b2cbeaaf8846818ca92ec5", "commit_rev": "35629b9c2d21ae7af4ec8ebdcec1c2c21a042880"}
3 years, 8 months ago
(2017-04-21 14:06:58 UTC)
#40
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492783382736660,
"parent_rev": "402945dae4329cd6e3b2cbeaaf8846818ca92ec5", "commit_rev":
"35629b9c2d21ae7af4ec8ebdcec1c2c21a042880"}
commit-bot: I haz the power
Description was changed from ========== Use {To/From}JavaTime for policy timestamps Use base::Time::ToJavaTime() and base::Time::FromJavaTime() for ...
3 years, 8 months ago
(2017-04-21 14:07:10 UTC)
#41
Message was sent while issue was closed.
Description was changed from
==========
Use {To/From}JavaTime for policy timestamps
Use base::Time::ToJavaTime() and base::Time::FromJavaTime() for
conversions between base::Time and timestamps in the policy protos.
No change of behavior should be introduced, it's just a refactoring.
BUG=701045
TEST=existing tests
==========
to
==========
Use {To/From}JavaTime for policy timestamps
Use base::Time::ToJavaTime() and base::Time::FromJavaTime() for
conversions between base::Time and timestamps in the policy protos.
No change of behavior should be introduced, it's just a refactoring.
BUG=701045
TEST=existing tests
Review-Url: https://codereview.chromium.org/2830033003
Cr-Commit-Position: refs/heads/master@{#466325}
Committed:
https://chromium.googlesource.com/chromium/src/+/35629b9c2d21ae7af4ec8ebdcec1...
==========
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/35629b9c2d21ae7af4ec8ebdcec1c2c21a042880
3 years, 8 months ago
(2017-04-21 14:07:15 UTC)
#42
Issue 2830033003: Use {To/From}JavaTime for policy timestamps
(Closed)
Created 3 years, 8 months ago by emaxx
Modified 3 years, 8 months ago
Reviewers: Thiemo Nagel
Base URL:
Comments: 12