Show time in hours, minutes and seconds in chrome://memory-internals

Show time in hours, minutes and seconds in chrome://memory-internals

BUG=279653
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260082

[ltilve, 2014-03-21 19:07:34]

Added a new secondsToHMS(totalSeconds) function to produce a readable string to
represent the equivalent to a total of seconds.
I'm using this function on time measures representations at
chrome://memory-internals.

[peria, 2014-03-23 15:16:27]

lgtm with nits.
thank you for the fixes.

https://codereview.chromium.org/208833003/diff/1/ui/webui/resources/js/util.js
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/208833003/diff/1/ui/webui/resources/js/util.j...
ui/webui/resources/js/util.js:440: var sec = Math.floor(totalSeconds % 3600 %
60);
remove "% 3600"

[ltilve, 2014-03-23 20:40:18]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-23 20:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/20001

[commit-bot: I haz the power, 2014-03-23 21:06:57]

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

[commit-bot: I haz the power, 2014-03-23 21:06:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[ltilve, 2014-03-24 08:21:34]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-24 08:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/20001

[commit-bot: I haz the power, 2014-03-24 09:01:11]

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

[commit-bot: I haz the power, 2014-03-24 09:01:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[ltilve, 2014-03-24 09:43:52]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-24 09:44:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/20001

[commit-bot: I haz the power, 2014-03-24 10:13:48]

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

[commit-bot: I haz the power, 2014-03-24 10:13:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[ltilve, 2014-03-24 16:59:24]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-24 16:59:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/20001

[commit-bot: I haz the power, 2014-03-24 17:29:53]

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

[commit-bot: I haz the power, 2014-03-24 17:29:53]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[ltilve, 2014-03-25 08:15:49]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-25 08:16:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/20001

[ltilve, 2014-03-25 08:38:25]

The CQ bit was unchecked by ltilve@igalia.com

[ltilve, 2014-03-25 08:48:16]

Adding more reviewers, as the chromium_presubmit bot was failing due to the need
of validation from an ui/webui/resources/js/util.js owner.

[arv (Not doing code reviews), 2014-03-25 16:34:39]

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:436: {
{ on previous line

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:441: return ((hour > 0 ? (hour + (hour > 1 ? '
hours ':' hour ')) : '') +
Maybe use Intl.DateTimeFormat instead?

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:441: return ((hour > 0 ? (hour + (hour > 1 ? '
hours ':' hour ')) : '') +
too many parentheses (the return expression is wrapped in one which is not
needed)

return (expr);

=>

return expr;

[Bernhard Bauer, 2014-03-25 16:36:10]

Urgh, forgot to publish comments :-(

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:430: * Produces a readable HH hours MM min. SS
sec. string representing
Nit: This sentence reads a big bumpy. How about "[...] a readable string in the
format '<HH> hours <MM> min. <SS> sec.' [...]"?

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:436: {
Opening braces goes on the previous line.

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:441: return ((hour > 0 ? (hour + (hour > 1 ? '
hours ':' hour ')) : '') +
This is of course an i18n nightmare. But the page where it is included is also
not i18nized, so I guess functionality-wise it's okay. I'm just a bit worried
about adding this method to a general utility library (and Web UI in general is
definitely i18n-aware). Maybe it would actually be better to move this back to
memory_internals.js?

[arv (Not doing code reviews), 2014-03-25 16:38:14]

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/208833003/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:441: return ((hour > 0 ? (hour + (hour > 1 ? '
hours ':' hour ')) : '') +
On 2014/03/25 16:36:10, Bernhard Bauer wrote:
> This is of course an i18n nightmare. But the page where it is included is also
> not i18nized, so I guess functionality-wise it's okay. I'm just a bit worried
> about adding this method to a general utility library (and Web UI in general
is
> definitely i18n-aware). Maybe it would actually be better to move this back to
> memory_internals.js?

I didn't even notice that this was in util.js

Please move to another file. util.js is used in a lot of places.

[ltilve, 2014-03-25 23:32:27]

Thanks for the reviews. I have applied the suggested modifications, moving the
function to memory_internals.js, omitting the Intl change for the reason
mentioned wrt the rest of the page not having i18n support.

[arv (Not doing code reviews), 2014-03-26 00:12:44]

On 2014/03/25 23:32:27, ltilve wrote:
> Thanks for the reviews. I have applied the suggested modifications, moving the
> function to memory_internals.js, omitting the Intl change for the reason
> mentioned wrt the rest of the page not having i18n support.

I was thinking that using Intl.DataFormat('en') might lead to code
simplification too.

[ltilve, 2014-03-26 01:24:31]

I have some doubt about this. IMHO, as we would still need to calculate the
total number of hours both for the uptime or for the open tabs, I think that we
would have to do in any case the calculation like Math.floor(totalSeconds /
3600), so I guess we couldn't directly use a date formatter.

I could still get the minutes/seconds for the return string creating a new
date=Date(totalSeconds*1000) and getting date.getMinutes()/date.getSeconds(),
but I'm not sure it's worth.

More code simplification could be done if the format is always like HHH:MM:SS,
but I was trying to get the string to be as short as possible when there were
only seconds or minutes.

In any case if you consider better some of these changes just tell me.

[Bernhard Bauer, 2014-03-26 12:41:09]

https://codereview.chromium.org/208833003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/memory_internals/memory_internals.js (right):

https://codereview.chromium.org/208833003/diff/70001/chrome/browser/resources...
chrome/browser/resources/memory_internals/memory_internals.js:164: function
secondsToHMS(totalSeconds) {
Move this into the anonymous function on lines 10 - 149?

[arv (Not doing code reviews), 2014-03-26 14:19:59]

LGTM

[ltilve, 2014-03-26 19:22:07]

https://codereview.chromium.org/208833003/diff/70001/chrome/browser/resources...
File chrome/browser/resources/memory_internals/memory_internals.js (right):

https://codereview.chromium.org/208833003/diff/70001/chrome/browser/resources...
chrome/browser/resources/memory_internals/memory_internals.js:164: function
secondsToHMS(totalSeconds) {
On 2014/03/26 12:41:09, Bernhard Bauer wrote:
> Move this into the anonymous function on lines 10 - 149?

Done.

[ltilve, 2014-03-27 20:27:16]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-27 20:28:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/160001

[commit-bot: I haz the power, 2014-03-28 02:02:08]

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

[commit-bot: I haz the power, 2014-03-28 02:02:10]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[ltilve, 2014-03-28 07:02:24]

The CQ bit was checked by ltilve@igalia.com

[commit-bot: I haz the power, 2014-03-28 07:03:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ltilve@igalia.com/208833003/160001

[commit-bot: I haz the power, 2014-03-28 07:06:02]

Change committed as 260082