DevTools: Server Timing values should be in milliseconds

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

Index: third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
diff --git a/third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js b/third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
index 9c7a27733854dd4a36ae3e837357bf695995deeb..b51a65013a0672696cdc4376b7366d4bb9159ab2 100644
--- a/third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
+++ b/third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js
@@ -273,8 +273,9 @@ Network.RequestTimingView = class extends UI.VBox {
       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 + '%';
@@ -283,8 +284,8 @@ Network.RequestTimingView = class extends UI.VBox {
           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.

     }
 
     /**