Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689833002:
DevTools: Server Timing values should be in milliseconds (Closed)
|
Created:
3 years, 10 months ago by paulirish Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Server Timing values should be in milliseconds
This is a breaking change. This feature landed in August, but just announced
this week (twitter.com/paul_irish/status/829090506084749312).
Users discovered the original patch used seconds values, but the Server
Timing spec dictates these as DOMHighResTimeStamp which are milliseconds.
BUG=594800
Review-Url: https://codereview.chromium.org/2689833002
Cr-Commit-Position: refs/heads/master@{#450244}
Committed: https://chromium.googlesource.com/chromium/src/+/968553977ba5cf51409101104a51431d60b051cd
Patch Set 1 #
Total comments: 10
Patch Set 2 : handle undefined values #
Total comments: 5
Patch Set 3 : nullable value. #Patch Set 4 : nullable value. #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== DevTools: Server Timing values should be in milliseconds BUG=594800 ========== to ========== DevTools: Server Timing values should be in milliseconds This is a breaking change. This feature landed in August, but just announced this week (twitter.com/paul_irish/status/829090506084749312). Users discovered the original patch used seconds values, but the Server Timing spec dictates these as DOMHighResTimeStamp which are milliseconds. BUG=594800 ==========
paulirish@chromium.org changed reviewers: + caseq@chromium.org, igrigorik@chromium.org
Earlier discussion was here: https://github.com/ChromeDevTools/devtools-frontend/issues/14 igrigorik, can I get your LGTM? I'll also reach out to the folks who wrote modules for ServerTiming this week (there's a few!) caseq, PTAL.
The CQ bit was checked by paulirish@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:278: if (serverTimingValue && left >= 0) { // don't chart values too big or too small can we rather have everything that depens on serverTimingValue being there (and being a number) under if (typeof serverTimingValue === 'number')?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/10 23:51:07, paulirish wrote: > Earlier discussion was here: > https://github.com/ChromeDevTools/devtools-frontend/issues/14 > > igrigorik, can I get your LGTM? In terms of functionality, LGTM -- thanks for the quick fix! I'll defer to caseq on the implementation. :-)
allada@chromium.org changed reviewers: + allada@chromium.org
https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:276: var serverTimingValue = serverTiming.value / 1000; // Server Timing values are in ms 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... https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:288: label.textContent = Number.secondsToString(serverTimingValue, true); I suggest using Number.millisToString and not do the "/ 1000" above instead.
allada, questions for you.. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:276: var serverTimingValue = serverTiming.value / 1000; // Server Timing values are in ms On 2017/02/11 at 06:21:58, allada wrote: > 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... 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. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:288: label.textContent = Number.secondsToString(serverTimingValue, true); On 2017/02/11 at 06:21:58, allada wrote: > I suggest using Number.millisToString and not do the "/ 1000" above instead. Was originally going to do that but the rest of this file uses secondsToString so it seemed better to keep it consistent. wdyt?
lmk if you have questions. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:276: var serverTimingValue = serverTiming.value / 1000; // Server Timing values are in ms On 2017/02/12 01:16:13, paulirish wrote: > On 2017/02/11 at 06:21:58, allada wrote: > > 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... > > 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. 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. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:288: label.textContent = Number.secondsToString(serverTimingValue, true); On 2017/02/12 01:16:13, paulirish wrote: > On 2017/02/11 at 06:21:58, allada wrote: > > I suggest using Number.millisToString and not do the "/ 1000" above instead. > > Was originally going to do that but the rest of this file uses secondsToString > so it seemed better to keep it consistent. wdyt? 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().
thanks for the feedback. ptal https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:276: var serverTimingValue = serverTiming.value / 1000; // Server Timing values are in ms On 2017/02/13 at 16:57:33, allada wrote: > > 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. 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. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:278: if (serverTimingValue && left >= 0) { // don't chart values too big or too small On 2017/02/11 at 01:05:18, caseq wrote: > can we rather have everything that depens on serverTimingValue being there (and being a number) under if (typeof serverTimingValue === 'number')? yup, done. https://codereview.chromium.org/2689833002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:288: label.textContent = Number.secondsToString(serverTimingValue, true); > 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(). sg, done.
wdyt? https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:277: if (typeof serverTiming.value !== 'number') Can we do this instead? if (serverTiming.value === null) https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:45: if (typeof value === 'number' || value === undefined) This can send in 'undefined' for value when null and number are only allowed. Lets do something more like this: if (typeof value !== 'number' || value === undefined) value = null; This should allow us to obtain a metric of null even if something invalid was passed for the value. (we'd get a key, but no value) wdyt?
lgtm https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:45: if (typeof value === 'number' || value === undefined) This will be always true due to lines 42 & 43. I guess you may want to check for NaN.
null values rather than undefined preferred now. https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:277: if (typeof serverTiming.value !== 'number') On 2017/02/14 at 00:00:24, allada wrote: > Can we do this instead? > > if (serverTiming.value === null) yup https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/2689833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:45: if (typeof value === 'number' || value === undefined) On 2017/02/14 at 00:00:24, allada wrote: > This can send in 'undefined' for value when null and number are only allowed. > > Lets do something more like this: > > if (typeof value !== 'number' || value === undefined) > value = null; > > This should allow us to obtain a metric of null even if something invalid was passed for the value. (we'd get a key, but no value) > > wdyt? yup sg. done.
The CQ bit was checked by paulirish@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from igrigorik@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2689833002/#ps60001 (title: "nullable value.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487039874032100,
"parent_rev": "1f7c5c5b93aa9caff5c35fed4b85063054bc1441", "commit_rev":
"968553977ba5cf51409101104a51431d60b051cd"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Server Timing values should be in milliseconds This is a breaking change. This feature landed in August, but just announced this week (twitter.com/paul_irish/status/829090506084749312). Users discovered the original patch used seconds values, but the Server Timing spec dictates these as DOMHighResTimeStamp which are milliseconds. BUG=594800 ========== to ========== DevTools: Server Timing values should be in milliseconds This is a breaking change. This feature landed in August, but just announced this week (twitter.com/paul_irish/status/829090506084749312). Users discovered the original patch used seconds values, but the Server Timing spec dictates these as DOMHighResTimeStamp which are milliseconds. BUG=594800 Review-Url: https://codereview.chromium.org/2689833002 Cr-Commit-Position: refs/heads/master@{#450244} Committed: https://chromium.googlesource.com/chromium/src/+/968553977ba5cf51409101104a51... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/968553977ba5cf51409101104a51... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
