DevTools: Server Timing values should be in milliseconds

third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 255 matching lines @@
      * @param {!SDK.ServerTiming} serverTiming
      * @param {number} right
      */
     function addTiming(serverTiming, right) {
       var colorGenerator = new PerfUI.FlameChart.ColorGenerator({min: 0, max: 360, count: 36}, {min: 50, max: 80}, 80);
       var isTotal = serverTiming.metric.toLowerCase() === 'total';
       var tr = tableElement.createChild('tr', isTotal ? 'network-timing-footer' : '');
       var metric = tr.createChild('td', 'network-timing-metric');
       metric.createTextChild(serverTiming.description || serverTiming.metric);
       var row = tr.createChild('td').createChild('div', 'network-timing-row');
-      var left = scale * (endTime - startTime - serverTiming.value);
-      if (serverTiming.value && left >= 0) {  // don't chart values too big or too small
+      var serverTimingValue = serverTiming.value / 1000; // Server Timing values are in ms

[allada, 2017/02/11 06:21:58]

serverTiming.value may be null. This should have been caught by closure, but the
type checking is not strict enough in closure. It may be worth either fixing or
putting a comment at this line stating that closure looses the types or forcing
the types:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

or just allowing in the constructor for the value here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

[paulirish, 2017/02/12 01:16:13]

true. 
are you OK with caseq's suggestion below? I have to check the regex but we
should definitely be careful in handling this user-defined value.

[allada, 2017/02/13 16:57:33]

Yes. I am mostly worried about the fact that the JSDoc shows the value as
not-null but it is being a possible null value, but we are checking it here...
we are not using closure to do what it is designed to do.

I already checked the RegExp, it appears fine... if there's a number there it
matches and properly parses out valid numbers (however it may be Infinity if the
number is too big) but if it doesn't exist or is not valid it should be null.

It's important to remember that the value is optional per line: 287.

[paulirish, 2017/02/13 22:18:46]

sounds good. The regex can end up returning an undefined value (but not null),
so I've handled that accordingly. Also updated the JSdoc to not lie.

+      var left = scale * (endTime - startTime - serverTimingValue);
+      if (serverTimingValue && left >= 0) {  // don't chart values too big or too small

[caseq, 2017/02/11 01:05:18]

can we rather have everything that depens on serverTimingValue being there (and
being a number) under if (typeof serverTimingValue === 'number')?

[paulirish, 2017/02/13 22:18:46]

yup, done.

         var bar = row.createChild('span', 'network-timing-bar server-timing');
         bar.style.left = left + '%';
         bar.style.right = right + '%';
         bar.textContent = '\u200B';  // Important for 0-time items to have 0 width.
         if (!isTotal)
           bar.style.backgroundColor = colorGenerator.colorForID(serverTiming.metric);
       }
       var label = tr.createChild('td').createChild('div', 'network-timing-bar-title');
-      if (typeof serverTiming.value === 'number')  // a metric timing value is optional
-        label.textContent = Number.secondsToString(serverTiming.value, true);
+      if (typeof serverTimingValue === 'number')  // a metric timing value is optional
+        label.textContent = Number.secondsToString(serverTimingValue, true);

[allada, 2017/02/11 06:21:58]

I suggest using Number.millisToString and not do the "/ 1000" above instead.

[paulirish, 2017/02/12 01:16:13]

Was originally going to do that but the rest of this file uses secondsToString
so it seemed better to keep it consistent. wdyt?

[allada, 2017/02/13 16:57:33]

In this case, I think it'd be better to use millisToString() because it makes it
much more clean on this line that the value is expected to be millis not seconds
and divided somewhere else. Also, secondsToString() is just taking the input
value and multiplying it by 1000 then using millisToString().

[paulirish, 2017/02/13 22:18:46]

sg, done.

     }
 
     /**
      * param {string} title
      */
     function createHeader(title) {
       var dataHeader = tableElement.createChild('tr', 'network-timing-table-header');
       dataHeader.createChild('td').createTextChild(title);
       dataHeader.createChild('td').createTextChild('');
       dataHeader.createChild('td').createTextChild(Common.UIString('TIME'));
@@ skipping 48 matching lines @@
 };
 
 Network.RequestTimingView.ConnectionSetupRangeNames = new Set([
   Network.RequestTimeRangeNames.Queueing, Network.RequestTimeRangeNames.Blocking,
   Network.RequestTimeRangeNames.Connecting, Network.RequestTimeRangeNames.DNS, Network.RequestTimeRangeNames.Proxy,
   Network.RequestTimeRangeNames.SSL
 ]);
 
 /** @typedef {{name: !Network.RequestTimeRangeNames, start: number, end: number}} */
 Network.RequestTimeRange;