Display "(local time)" label after start/end time of the build

Display "(local time)" label after start/end time of the build

R=hinoka@chromium.org
BUG=691493
Review-Url: https://codereview.chromium.org/2691003002
Committed: https://github.com/luci/luci-go/commit/b517e730515610038aa8ef9c6d9c4ef2c5d4c05c

[Sergiy Byelozyorov, 2017-02-13 10:17:35]

Description was changed from

==========
Display "(local time)" label after start/end time of the build

R=hinoka@chromium.org
BUG=
==========

to

==========
Display "(local time)" label after start/end time of the build

R=hinoka@chromium.org
BUG=691493
==========

[Sergiy Byelozyorov, 2017-02-13 10:17:39]

The CQ bit was checked by sergiyb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 10:17:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 10:22:15]

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

[commit-bot: I haz the power, 2017-02-13 10:22:16]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/344fcc1f411e3f10)

[hinoka, 2017-02-13 19:59:30]

Actually by default go renders UTC time and uses javascript to convert it to
local time.  If the javascript fails (unlikely, but still something to keep in
mind), this would display UTC appended with local time.

Instead this should go here:
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/milo/ap...

[hinoka, 2017-02-15 21:50:06]

This would also work poorly if javascript was turned off.  It'd display UTC but
say it's local time.

How about showing the time offset instead or in addition to?

[Sergiy Byelozyorov, 2017-02-16 12:13:14]

On 2017/02/15 21:50:06, hinoka wrote:
> This would also work poorly if javascript was turned off.  It'd display UTC
but
> say it's local time.
> 
> How about showing the time offset instead or in addition to?

Done.

[Sergiy Byelozyorov, 2017-02-16 12:13:33]

On 2017/02/16 12:13:14, Sergiy Byelozyorov wrote:
> On 2017/02/15 21:50:06, hinoka wrote:
> > This would also work poorly if javascript was turned off.  It'd display UTC
> but
> > say it's local time.
> > 
> > How about showing the time offset instead or in addition to?
> 
> Done.

Also updated tainted version:
https://1380-df83e09-tainted-sergiyb-dot-luci-milo.appspot.com/buildbot/chrom....

[Sergiy Byelozyorov, 2017-02-21 14:35:58]

gentle ping

[hinoka, 2017-02-22 17:57:06]

Hi, sorry for the delay

This change was a little contentious so we sent out some straw polls and surveys
to gather some data.  The results have come back.

https://docs.google.com/a/google.com/forms/d/1f0EECtt6twA7njEw82Bil63gHVAttWC...
https://docs.google.com/a/google.com/forms/d/126AEXrgUO8fC9xsgUatE7jRnE4qLeHn...

The results from polling a few infra members (And "Android users" just for good
measure) was that users preferred either (local time) like the original or
(PST).  What we found out was that very few people preferred the time offsets,
and obviously no one likes UTC.

Named timezones are good but they require either 30KB+ worth of javascript
library to pull off, or some very complex logic, so that's sort of a
non-starter.

So, let's change this back to appending (local time) instead of a time offset
(but with javascript).

[Sergiy Byelozyorov, 2017-02-22 19:22:02]

Did your poll include people from Europe? We like UTC because it's just 1-2
hours away :-).

On Wed, 22 Feb 2017, 18:57 , <hinoka@chromium.org> wrote:

> Hi, sorry for the delay
>
> This change was a little contentious so we sent out some straw polls and
> surveys
> to gather some data. The results have come back.
>
>
>
https://docs.google.com/a/google.com/forms/d/1f0EECtt6twA7njEw82Bil63gHVAttWC...
>
>
https://docs.google.com/a/google.com/forms/d/126AEXrgUO8fC9xsgUatE7jRnE4qLeHn...
>
> The results from polling a few infra members (And "Android users" just for
> good
> measure) was that users preferred either (local time) like the original or
> (PST). What we found out was that very few people preferred the time
> offsets,
> and obviously no one likes UTC.
>
> Named timezones are good but they require either 30KB+ worth of javascript
> library to pull off, or some very complex logic, so that's sort of a
> non-starter.
>
> So, let's change this back to appending (local time) instead of a time
> offset
> (but with javascript).
>
> https://codereview.chromium.org/2691003002/
>
-- 
Sergiy Byelozyorov | Software Engineer | sergiyb@google.com

Google Germany GmbH
Erika-Mann-Strasse 33
80636 München

AG Hamburg, HRB 86891 | Sitz der Gesellschaft: Hamburg | Geschäftsführer:
Matthew Scott Sucherman, Paul Terence Manicle

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hinoka, 2017-02-22 19:26:16]

1-2 hours is not free either, someone would have to think about if you're on
CEST or CET (same problem with PST/PDT...) and often run into off-by-one errors.

We might have to bite the bullet one day and just implement full timezone
support (we have an okay from estaab@ to import moment.js as long as caching is
setup correctly).  But for the moment (pun unintended) just appending (local) at
the end is fine.

[Sergiy Byelozyorov, 2017-02-28 11:47:30]

On 2017/02/22 19:26:16, hinoka wrote:
> 1-2 hours is not free either, someone would have to think about if you're on
> CEST or CET (same problem with PST/PDT...) and often run into off-by-one
errors.
> 
> We might have to bite the bullet one day and just implement full timezone
> support (we have an okay from estaab@ to import moment.js as long as caching
is
> setup correctly).  But for the moment (pun unintended) just appending (local)
at
> the end is fine.

Done. PTAL

[nodir, 2017-02-28 15:56:22]

On 2017/02/28 11:47:30, Sergiy Byelozyorov wrote:
> On 2017/02/22 19:26:16, hinoka wrote:
> > 1-2 hours is not free either, someone would have to think about if you're on
> > CEST or CET (same problem with PST/PDT...) and often run into off-by-one
> errors.
> > 
> > We might have to bite the bullet one day and just implement full timezone
> > support (we have an okay from estaab@ to import moment.js as long as caching
> is
> > setup correctly).  But for the moment (pun unintended) just appending
(local)
> at
> > the end is fine.
> 
> Done. PTAL

I still see (local time) instead of (PST)

[Sergiy Byelozyorov, 2017-02-28 16:12:22]

On 2017/02/28 15:56:22, nodir wrote:
> On 2017/02/28 11:47:30, Sergiy Byelozyorov wrote:
> > On 2017/02/22 19:26:16, hinoka wrote:
> > > 1-2 hours is not free either, someone would have to think about if you're
on
> > > CEST or CET (same problem with PST/PDT...) and often run into off-by-one
> > errors.
> > > 
> > > We might have to bite the bullet one day and just implement full timezone
> > > support (we have an okay from estaab@ to import moment.js as long as
caching
> > is
> > > setup correctly).  But for the moment (pun unintended) just appending
> (local)
> > at
> > > the end is fine.
> > 
> > Done. PTAL
> 
> I still see (local time) instead of (PST)

Yes, that's what Ryan suggested in #12 and #14, isn't it?

[hinoka, 2017-02-28 18:10:41]

Yes that is what I suggested.  We can pursue named timezones (and full timezone
support) in the future, this will do for now.

lgtm + comments.

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
File milo/appengine/frontend/static/common/js/time.js (right):

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/static/common/js/time.js:25: var offsetSign = '';
Don't need this anymore

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/static/common/js/time.js:32: if (offset < 0) {
Or this

[Sergiy Byelozyorov, 2017-03-01 08:34:52]

The CQ bit was checked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2017-03-01 08:34:53]

The CQ bit was unchecked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2017-03-01 08:35:05]

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
File milo/appengine/frontend/static/common/js/time.js (right):

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/static/common/js/time.js:25: var offsetSign = '';
On 2017/02/28 18:10:41, hinoka wrote:
> Don't need this anymore

Done.

https://codereview.chromium.org/2691003002/diff/60001/milo/appengine/frontend...
milo/appengine/frontend/static/common/js/time.js:32: if (offset < 0) {
On 2017/02/28 18:10:41, hinoka wrote:
> Or this

Done.

[Sergiy Byelozyorov, 2017-03-01 08:35:08]

The CQ bit was checked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2017-03-01 08:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/2691003002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-03-01 08:35:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 08:41:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488357308331620,
"parent_rev": "c8d155bc3c1b87edffe942642b4099f1958cb780", "commit_rev":
"b517e730515610038aa8ef9c6d9c4ef2c5d4c05c"}

[commit-bot: I haz the power, 2017-03-01 08:41:09]

Description was changed from

==========
Display "(local time)" label after start/end time of the build

R=hinoka@chromium.org
BUG=691493
==========

to

==========
Display "(local time)" label after start/end time of the build

R=hinoka@chromium.org
BUG=691493

Review-Url: https://codereview.chromium.org/2691003002
Committed:
https://github.com/luci/luci-go/commit/b517e730515610038aa8ef9c6d9c4ef2c5d4c05c
==========

[commit-bot: I haz the power, 2017-03-01 08:41:11]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/b517e730515610038aa8ef9c6d9c4ef2c5d4c05c

[mithro, 2017-03-02 06:29:22]

tansell@chromium.org changed reviewers:
+ tansell@chromium.org

[mithro, 2017-03-02 06:29:23]

https://codereview.chromium.org/2691003002/diff/80001/milo/appengine/frontend...
File milo/appengine/frontend/static/common/js/time.js (right):

https://codereview.chromium.org/2691003002/diff/80001/milo/appengine/frontend...
milo/appengine/frontend/static/common/js/time.js:32: s += t.toLocaleTimeString()
+ ' (local time)';
This text is not useful, who's local time?

Please do something like

s += t.toLocaleTimeString() + '('+ t.toISOString() + ')'

Or better yet, just always use the ISO string.

[Sergiy Byelozyorov, 2017-03-02 08:59:29]

On 2017/03/02 06:29:23, mithro wrote:
>
https://codereview.chromium.org/2691003002/diff/80001/milo/appengine/frontend...
> File milo/appengine/frontend/static/common/js/time.js (right):
> 
>
https://codereview.chromium.org/2691003002/diff/80001/milo/appengine/frontend...
> milo/appengine/frontend/static/common/js/time.js:32: s +=
t.toLocaleTimeString()
> + ' (local time)';
> This text is not useful, who's local time?

IMHO, local time means time used where your computer is located (probably
depends on browser settings).

> Please do something like
> 
> s += t.toLocaleTimeString() + '('+ t.toISOString() + ')'
> 
> Or better yet, just always use the ISO string.

toISOString always returns UTC, e.g. 2017-03-02T08:53:03.105Z, and according to
the survey done by Ryan, this was least popular preference. Although purely
subjective, it seems harder to read because there is no space between date, time
and timezone, and because some people may not be aware that "Z" means Zulu, and
thus Zulu means UTC. Perhaps a better option would be to add actual timezone
abbreviations as Ryan suggested in the comments to this CL. Unfortunately I
don't have time to do this. If this is super-annoying, feel free to revert.