Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1794783006:
Add Server-Timing support to devtools (Closed)
|
Created:
4 years, 9 months ago by sroussey Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, igrigorik_gmail.com, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Server-Timing support to devtools
BUG=594800
Committed: https://crrev.com/b9174da95316a0d17f6868a31780b046c5a374d2
Cr-Commit-Position: refs/heads/master@{#411914}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Updated version #
Total comments: 15
Patch Set 3 : Redone based on comments #Patch Set 4 : Fix with file name change #
Total comments: 4
Patch Set 5 : Version with bar chart for server timing #
Total comments: 2
Patch Set 6 : Fix small issues #Patch Set 7 : Apply feedback, don't have bars where the data is out of bounds, and handle comma deliminated heade… #
Total comments: 16
Patch Set 8 : Update with caseq suggestions #Patch Set 9 : Update with caseq suggestions #
Total comments: 3
Patch Set 10 : updated #Patch Set 11 : rebased #Patch Set 12 : fix trailing whitespace #Patch Set 13 : really remove whitespace. sigh. #
Total comments: 4
Patch Set 14 : Remove target #Patch Set 15 : fix line-endings again #
Messages
Total messages: 69 (34 generated)
sroussey@gmail.com changed reviewers: + paulirish@chromium.com, pfeldman@chromium.com
This takes Server-Timing headers and appends them to the RequestTimingView. If I can upload images here, let me know so I can post screenshots.
On 2016/03/13 at 22:13:23, sroussey wrote: > This takes Server-Timing headers and appends them to the RequestTimingView. If I can upload images here, let me know so I can post screenshots. Whoa nice. We'll take a look! Thanks Steve
Description was changed from ========== Add Server-Timing support to devtools BUG= ========== to ========== Add Server-Timing support to devtools BUG=594800 ==========
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:287: serverHeader.createChild("td").createTextChild("Server-Timing"); User-visible strings should be wrapped with WebInspector.UIString() https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:289: serverHeader.createChild("td").createTextChild("TIME"); ditto https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:291: var addTiming = (serverTiming) => { Please declare as a named function. /** * @param ... */ function addTiming(serverTiming) { } https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js (right): https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:2: * Copyright (C) 2016 Steven Roussey. All rights reserved. Could you use a new 3 line header style? Also, you need to be in the AUTHORS list to land your patch: https://www.chromium.org/developers/contributing-code https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:51: this._serverTimings = this._headers.map(item => this._extractMetric(item.value), this).filter(item => item); This is hard to read - it might be easier to implement what you need in the extractMetric (including initialization). https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:61: this._headers = headers.filter(item => item.name.toLowerCase() == 'server-timing'); == -> === https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:65: this._headers.sort((a, b) => a.value.toLowerCase().compareTo(b.value.toLowerCase())); Why do you have to sort it?
On 2016/03/14 at 22:54:35, pfeldman wrote: > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:287: serverHeader.createChild("td").createTextChild("Server-Timing"); > User-visible strings should be wrapped with WebInspector.UIString() > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:289: serverHeader.createChild("td").createTextChild("TIME"); > ditto > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:291: var addTiming = (serverTiming) => { > Please declare as a named function. > > /** > * @param ... > */ > function addTiming(serverTiming) { > } > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js (right): > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:2: * Copyright (C) 2016 Steven Roussey. All rights reserved. > Could you use a new 3 line header style? > > Also, you need to be in the AUTHORS list to land your patch: https://www.chromium.org/developers/contributing-code > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:51: this._serverTimings = this._headers.map(item => this._extractMetric(item.value), this).filter(item => item); > This is hard to read - it might be easier to implement what you need in the extractMetric (including initialization). > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:61: this._headers = headers.filter(item => item.name.toLowerCase() == 'server-timing'); > == -> === > > https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:65: this._headers.sort((a, b) => a.value.toLowerCase().compareTo(b.value.toLowerCase())); > Why do you have to sort it? Also, now I just sort the metric name so it is more obvious.
Oh, it doesn't show here, but on the links above I had added additional comments.
https://codereview.chromium.org/1794783006/diff/20001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1794783006/diff/20001/AUTHORS#newcode577 AUTHORS:577: Steven Roussey <sroussey@gmail.com> As Paul mentioned, you might need to move this into the UIString change. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:284: if (serverTimings) { [style] Blink's code style suggests to prefer early returns: if (!serverTimings) return tableElement; https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:296: function addTiming(serverTiming) { [style] { goes next line. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/networkPanel.css (right): https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/networkPanel.css:103: background-image: linear-gradient(to right, #eee, #bbb, #eee); Please use flat #eee https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js (right): https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:21: var rawServerTimingHeaders = headers.filter(item => item.name.toLowerCase() === 'server-timing'); "server-timing" https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:25: var serverTimings = rawServerTimingHeaders.map(item => this._extractMetric(item.value), this); Iterating over array here and inlining _extractMetric into the loop would allow to save on another linear path below. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:27: serverTimings.sort((a, b) => a.metric.toLowerCase().compareTo(b.metric.toLowerCase())); I'd rather preserve the order of the timing sent by the server. That way it can be mind-mapped into something readable and waterfall-ish. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:37: if (valueString == '') === "" or even better: if (!valueString) https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:41: console.log("Failed parsing server-timing metric: ", valueString); We don't console.log into inspector, silent return here. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:67: WebInspector.ServerTiming = function(target, metric, value, description) Not sure you need WebInspector.ServerTimingParser - you could get away with just WebInspector.ServerTiming class and WebInspector.ServerTiming.parse(target, headers) method (extractMetric would just be inlined) You'd need to rename file though.
paulirish@chromium.org changed reviewers: + paulirish@chromium.org - pfeldman@chromium.com
https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:289: serverHeader.createChild("td").createTextChild(WebInspector.UIString("Server-Timing")); "Server Timing" https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:301: metric.createTextChild(serverTiming.description || serverTiming.metric); Would you want to see both metric name and optional description? https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:308: serverTimings.filter(item => item.metric.toLowerCase() == "total").forEach(addTiming); This does rely on convention that a metric with the name "total" is added, but the spec seems to suggestion that. So, OK.
lgtm. it would be nice to get a test for this one as a follow up. it might require building chromium though. https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:28: if (rawServerTimingHeaders.length === 0) nit: if (!rawServerTimingHeaders.length) https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:31: function createFromHeaderValue(valueString) Please add valueString jsdoc.
On 2016/03/21 at 18:26:47, pfeldman wrote: > lgtm. it would be nice to get a test for this one as a follow up. it might require building chromium though. > > https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): > > https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:28: if (rawServerTimingHeaders.length === 0) > nit: if (!rawServerTimingHeaders.length) > > https://codereview.chromium.org/1794783006/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:31: function createFromHeaderValue(valueString) > Please add valueString jsdoc. There is a new version here now.
caseq@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:290: serverHeader.createChild("td").createTextChild(""); nuke createTextChild() here? https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:304: var isTotal = serverTiming.metric == "total"; s/==/===/ Also, we use toLowerCase() when comparing with "total" below, shouldn't we do it here? https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:308: row = tr.createChild("td").createChild("div", "network-timing-row"); var row https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:309: left = scale * (endTime - startTime - serverTiming.value); var left https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:311: bar = row.createChild("span", "network-timing-bar server-timing"); var bar https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:313: bar.style.right = right + "%"; What's the value of right here? https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:319: if (serverTiming.value !== null) // a metric timing value is optional nit: check for typeof serverTiming.value === "number" instead. https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:323: serverTimings.filter(item => item.metric.toLowerCase() != "total").forEach(addTiming); style: !== https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:324: serverTimings.filter(item => item.metric.toLowerCase() == "total").forEach(addTiming); style: === https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/networkPanel.css (right): https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/networkPanel.css:179: overflow-x:hidden; style: space after : https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:22: * @param {!Array.<!WebInspector.NetworkRequest.NameValue>} headers nit: here and below, please use Array<> instead of Array.<> https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:27: var rawServerTimingHeaders = headers.filter(item => item.name.toLowerCase() === 'server-timing'); use "double quotes", please. https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:37: // Server-Timing = "Server-Timing" ":" #server-timing-metric Should we have a link to the w3c doc here instead? https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:48: var metricMatch, result = []; style: please put result on a separate line. https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:51: value = metricMatch[2], style: please split into separate var statements.
There is a new version to take of all the things caseq pointed out. https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:287: serverHeader.createChild("td").createTextChild("Server-Timing"); On 2016/03/14 at 22:54:34, pfeldman wrote: > User-visible strings should be wrapped with WebInspector.UIString() OK! You might look at other things in this file -- I copy-pasted. 8-) https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js (right): https://codereview.chromium.org/1794783006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:65: this._headers.sort((a, b) => a.value.toLowerCase().compareTo(b.value.toLowerCase())); On 2016/03/14 at 22:54:35, pfeldman wrote: > Why do you have to sort it? Related to a my experience writing a Server-Timing extension (https://chrome.google.com/webstore/detail/server-timing/lhebeopdclghecelfokfa...). I sort the metrics as I recommend naming them with prefixes (like data-db-mysql and data-kv-memcache, then this way, even though their names are different, all the similar things get put together. In my extension, when complete, the donut graphs will have sub-circles that help show the relative weight of data-* vs network-*, as well as mysql vs memcache. My plan is to make a blog post at the same time this goes live, and also release the related extension at the same time. https://codereview.chromium.org/1794783006/diff/20001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1794783006/diff/20001/AUTHORS#newcode577 AUTHORS:577: Steven Roussey <sroussey@gmail.com> On 2016/03/15 at 20:18:59, pfeldman wrote: > As Paul mentioned, you might need to move this into the UIString change. OK, I will re-submit both tonight. https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js (right): https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:25: var serverTimings = rawServerTimingHeaders.map(item => this._extractMetric(item.value), this); On 2016/03/15 at 20:18:59, pfeldman wrote: > Iterating over array here and inlining _extractMetric into the loop would allow to save on another linear path below. I can use reduce() with an array memo. +1 https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:27: serverTimings.sort((a, b) => a.metric.toLowerCase().compareTo(b.metric.toLowerCase())); On 2016/03/15 at 20:19:00, pfeldman wrote: > I'd rather preserve the order of the timing sent by the server. That way it can be mind-mapped into something readable and waterfall-ish. I guess this is based on my particular experience where they are not always coming out in the same order across different pages (and I can't reorder already sent headers in PHP). Imagine if the other items in this view changed order sometimes. Yikes. Also, this data is _not_ waterfall-like. They say what % of the time was done doing what. Nothing about the order thereof. They do not specify steps. Though if you wanted that, you could name metics in order: Server-Timing: step001-auth=30;"Authenticate" https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:41: console.log("Failed parsing server-timing metric: ", valueString); On 2016/03/15 at 20:18:59, pfeldman wrote: > We don't console.log into inspector, silent return here. OK, but we might want to remove the one I got the idea from... https://codereview.chromium.org/1794783006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ServerTimingParser.js:67: WebInspector.ServerTiming = function(target, metric, value, description) On 2016/03/15 at 20:19:00, pfeldman wrote: > Not sure you need WebInspector.ServerTimingParser - you could get away with just > > WebInspector.ServerTiming class and > WebInspector.ServerTiming.parse(target, headers) method (extractMetric would just be inlined) > > You'd need to rename file though. Yeah, I like that better now. https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:301: metric.createTextChild(serverTiming.description || serverTiming.metric); On 2016/03/20 at 20:16:27, paulirish wrote: > Would you want to see both metric name and optional description? I don't think so. A lot of people will have a metric of "mysql" and a description of "MySQL" https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:313: bar.style.right = right + "%"; On 2016/04/05 at 17:18:41, caseq wrote: > What's the value of right here? The right from the closure.
Thanks, looks mostly good to me, but still needs a bit of polish. https://codereview.chromium.org/1794783006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:285: var breakEl = tableElement.createChild("tr", "network-timing-table-header").createChild("td"); nit: breakElement, blink's code style doesn't favor abbreviations. https://codereview.chromium.org/1794783006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:313: bar.style.right = right + "%"; let's avoid using right from the closure, especially given the fact that it changes in the loop. please make sure the correct value for right is computed or passed explicitly. https://codereview.chromium.org/1794783006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/1794783006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:14: this._target = target; why do we need target here?
steven, do you want to finish this CL off?
Odd, I don't see my last code push. I'll check when I get home tonight. Perhaps I didn't actually push it. :/ Steven Roussey On Mon, Jul 11, 2016 at 3:43 PM, <paulirish@chromium.org> wrote: > steven, do you want to finish this CL off? > > https://codereview.chromium.org/1794783006/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Odd, I don't see my last code push. I'll check when I get home tonight. Perhaps I didn't actually push it. :/ Steven Roussey On Mon, Jul 11, 2016 at 3:43 PM, <paulirish@chromium.org> wrote: > steven, do you want to finish this CL off? > > https://codereview.chromium.org/1794783006/ > -- 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.
And then I updated the code, and didn't update here...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/08/03 at 00:02:57, sroussey wrote: > And then I updated the code, and didn't update here... I think I may need to rebase. It has been a while.
Yup, you'll need to rebase. But it's really straightforward. Conflicts in CSS and sdk/module.json Both trivial to resolve. Easiest way to rebase: git checkout master git pull origin master git checkout servertimingbranch git rebase master I just prefer this in contrast to using `git rebase-update` goodluck!
I'm getting some odd errors now: Runtime.js:72 GET http://localhost:9999/front_end/InspectorBackendCommands.js 404 (File not found)load @ Runtime.js:72loadResourcePromise @ Runtime.js:47loadScriptsPromise @ Runtime.js:134_loadScripts @ Runtime.js:771 Runtime.js:161 Empty response arrived for script 'http://localhost:9999/front_end/InspectorBackendCommands.js'evaluateScript @ Runtime.js:161scriptSourceLoaded @ Runtime.js:147 Runtime.js:72 GET http://localhost:9999/front_end/SupportedCSSProperties.js 404 (File not found)load @ Runtime.js:72loadResourcePromise @ Runtime.js:47loadScriptsPromise @ Runtime.js:134_loadScripts @ Runtime.js:771 Runtime.js:161 Empty response arrived for script 'http://localhost:9999/front_end/SupportedCSSProperties.js'evaluateScript @ Runtime.js:161scriptSourceLoaded @ Runtime.js:147 These files don't exist. This happens on master. I can't seem to reload a page in canary to get data into the inspector because of this.
I rebased. How do I send to test? I'm guessing you did last time. Do I use CQ dry run?
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...
On 2016/08/05 at 22:48:50, sroussey wrote: > I rebased. How do I send to test? I'm guessing you did last time. Do I use CQ dry run? I believe new contributors don't have permission, despite it looking like you do. :/ I just triggered the cq dry run for you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Below is the rejection from the presubmit bot; looks like some trailing whitespace. : ------------------------------------------------------------ ** Presubmit Warnings ** Found line ending with white spaces in: *************** third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:304 third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:329 third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:30 third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:52 ***************
The CQ bit was checked by sroussey@gmail.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sroussey@gmail.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/06 at 06:09:20, paulirish wrote: > Below is the rejection from the presubmit bot; looks like some trailing whitespace. : > > ------------------------------------------------------------ > > ** Presubmit Warnings ** > Found line ending with white spaces in: > > *************** > third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:304 > third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:329 > third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:30 > third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:52 > *************** Fixed it. I also was able to re-run the CQ dry run and all is green.
caseq, now back to you for review :)
Almost there! https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js (right): https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/RequestTimingView.js:330: serverTimings.filter(item => item.metric.toLowerCase() !== "total").forEach(item => addTiming(item, right)); let's have right explicitly computed somewhere on top level. relying on a variable definition within the loop is confusing. https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js (right): https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:7: * @param {!WebInspector.Target} target why pass target? https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:14: this._target = target; let's nuke it. https://codereview.chromium.org/1794783006/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ServerTiming.js:21: * @param {!WebInspector.Target} target ditto.
The CQ bit was checked by sroussey@gmail.com 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...
OK! Updated!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sroussey@gmail.com 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...
On 2016/08/11 at 05:16:37, sroussey wrote: > OK! Updated! And fixed the line-endings again...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/11 at 05:31:45, sroussey wrote: > On 2016/08/11 at 05:16:37, sroussey wrote: > > OK! Updated! > > And fixed the line-endings again... I don't understand what the one failure on mac_chromium_10.10_rel_ng is...
On 2016/08/12 at 03:43:18, sroussey wrote: > On 2016/08/11 at 05:31:45, sroussey wrote: > > On 2016/08/11 at 05:16:37, sroussey wrote: > > > OK! Updated! > > > > And fixed the line-endings again... > > I don't understand what the one failure on mac_chromium_10.10_rel_ng is... Same. Retrying that bot..
lgtm
The CQ bit was checked by caseq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1794783006/#ps280001 (title: "fix line-endings again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by paulirish@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add Server-Timing support to devtools BUG=594800 ========== to ========== Add Server-Timing support to devtools BUG=594800 Committed: https://crrev.com/b9174da95316a0d17f6868a31780b046c5a374d2 Cr-Commit-Position: refs/heads/master@{#411914} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/b9174da95316a0d17f6868a31780b046c5a374d2 Cr-Commit-Position: refs/heads/master@{#411914} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
