Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627073003:
[Devtools] Fixed url in Audits and added doctypes (Closed)
|
Created:
3 years, 11 months ago by allada Modified:
3 years, 11 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Fixed url in Audits
This patch fixes areas where NetworkRequest.url conversion was missed
because file lacked doctype.
R=dgozman,luoe
BUG=680054
Review-Url: https://codereview.chromium.org/2627073003
Cr-Commit-Position: refs/heads/master@{#444962}
Committed: https://chromium.googlesource.com/chromium/src/+/5b078604227b8dd8864657954161097abc3091ee
Patch Set 1 #
Total comments: 27
Patch Set 2 : Merge branch 'master' into AUDITRULES_TYPED #
Total comments: 2
Patch Set 3 : changes #Patch Set 4 : changes #Patch Set 5 : fixed audits #Messages
Total messages: 24 (11 generated)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
allada@chromium.org changed reviewers: + alph@chromium.org, caseq@chromium.org
PTL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:78: var output = {}; nit: can you plz use Map as assigning arbitrary keys to an object is not a good idea.
PTaL https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:78: var output = {}; On 2017/01/12 07:54:42, alph wrote: > nit: can you plz use Map as assigning arbitrary keys to an object is not a good > idea. I don't want to do this because this patch is not to refactor anything it's only to get stuff typed. I separated these two functions because it was one function with two different output types. I simply separated the single function into two functions.
Although cleaning up AuditsRules.js wasn't the original goal of this patch, removing the spread operator and the unused Audits.AuditRules.CSSRuleBase class would be great own their own. Your patch has added doctypes in places where they were needed, but a many of the existing doctypes in the file were incorrect before. After looking for ".url"s, I found another place where the NetworkRequest.url conversion might've been missed in the loadingFailed() of NetworkManager.js: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:75: Audits.AuditRules.getDomainToResourceUrlMap = function(requests, types) { I don't think this new function is needed. The places where it is used (lines 174, 241) only access the domain and get the length of the domain's requests array. Nothing seems to use the actual url's. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:95: * @param {!Array.<!SDK.NetworkRequest>} requests Remove period here and in all the places where there's ".<" https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:128: * @param {!SDK.NetworkRequest} request Missing @return here and in many other functions https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:550: allRules.push.apply(allRules, rules); I'm not sure how large the "rules" array gets, but apply with too many items may fail. Perhaps just for loop here? https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:884: /** @type {!Object<string,number>} */ Nit: space after comma https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1253: Audits.AuditRules.CSSRuleBase = class extends Audits.AuditRule { Not related to typing, but it looks like this class is never used anywhere. If you decide to include Audits cleanup in this CL about cleaning up Audits, I'd remove it now. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1458: * @param {!Array<!SDK.Cookie>} cookieArray @return {number} https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1468: * @param {!Array<!SDK.Cookie>} cookieArray ditto https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1484: * @param {!{domain: string, avgCookieSize: number, maxCookieSize: number}} a typedef CookieStats? @return number https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1492: * @param {!{domain: string, avgCookieSize: number, maxCookieSize: number}} a ditto https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1509: cookies = []; I guess closure doesn't complain about this?
allada@chromium.org changed reviewers: + pfeldman@chromium.org
done. PTL https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:75: Audits.AuditRules.getDomainToResourceUrlMap = function(requests, types) { On 2017/01/13 08:05:22, luoe wrote: > I don't think this new function is needed. The places where it is used (lines > 174, 241) only access the domain and get the length of the domain's requests > array. Nothing seems to use the actual url's. line: 245 does. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:95: * @param {!Array.<!SDK.NetworkRequest>} requests On 2017/01/13 08:05:22, luoe wrote: > Remove period here and in all the places where there's ".<" This patch isn't here to fix legacy doctypes. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:128: * @param {!SDK.NetworkRequest} request On 2017/01/13 08:05:22, luoe wrote: > Missing @return here and in many other functions This patch isn't here to fix legacy doctypes. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:550: allRules.push.apply(allRules, rules); On 2017/01/13 08:05:21, luoe wrote: > I'm not sure how large the "rules" array gets, but apply with too many items may > fail. Perhaps just for loop here? Apply is better per kozyatinskiy@ https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:884: /** @type {!Object<string,number>} */ On 2017/01/13 08:05:22, luoe wrote: > Nit: space after comma Done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1253: Audits.AuditRules.CSSRuleBase = class extends Audits.AuditRule { On 2017/01/13 08:05:22, luoe wrote: > Not related to typing, but it looks like this class is never used anywhere. If > you decide to include Audits cleanup in this CL about cleaning up Audits, I'd > remove it now. I would rather not in this cl, I only want to protect our code from changes to code elsewhere letting closure catch it. I don't want to change any of the actual way things are done in this code since it's likely all getting removed. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1458: * @param {!Array<!SDK.Cookie>} cookieArray On 2017/01/13 08:05:22, luoe wrote: > @return {number} Done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1468: * @param {!Array<!SDK.Cookie>} cookieArray On 2017/01/13 08:05:22, luoe wrote: > ditto Done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1484: * @param {!{domain: string, avgCookieSize: number, maxCookieSize: number}} a On 2017/01/13 08:05:23, luoe wrote: > typedef CookieStats? > @return number Done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1509: cookies = []; On 2017/01/13 08:05:22, luoe wrote: > I guess closure doesn't complain about this? It can infer it from line 1500
https://codereview.chromium.org/2627073003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:297: /** @type {!Array<string>} */ Let's not enforce annotating inline objects and arrays at this point. Other annotations sound good.
done. https://codereview.chromium.org/2627073003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:297: /** @type {!Array<string>} */ On 2017/01/17 19:33:55, pfeldman wrote: > Let's not enforce annotating inline objects and arrays at this point. Other > annotations sound good. Acknowledged.
lgtm with comments https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:75: Audits.AuditRules.getDomainToResourceUrlMap = function(requests, types) { Line 245 is using "domain", the keys of the map, not the urls in the values. The values "domainToResourcesMap[domain]" is never used besides getting its length. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1509: cookies = []; I changed it to "cookies = [123]" and ran closure without any errors. If you decide to doctype new objects/arrays, let's add a doctype here too.
done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right): https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:75: Audits.AuditRules.getDomainToResourceUrlMap = function(requests, types) { On 2017/01/17 19:40:16, luoe wrote: > Line 245 is using "domain", the keys of the map, not the urls in the values. > The values "domainToResourcesMap[domain]" is never used besides getting its > length. Done. https://codereview.chromium.org/2627073003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1509: cookies = []; On 2017/01/17 19:40:16, luoe wrote: > I changed it to "cookies = [123]" and ran closure without any errors. If you > decide to doctype new objects/arrays, let's add a doctype here too. What dgozman@ and I agreed on is that we should doctype things that are to be passed into functions or used as arguments. Since "cookies" here is simply a temp variable used to be assigned to an already typed item but never actually used, I believe it would be overkill for what we are trying to protect against.
lgtm
Description was changed from ========== [Devtools] Fixed url in Audits and added doctypes This patch fixes areas where NetworkRequest.url conversion was missed because file lacked doctype. R=dgozman,luoe BUG=680054 ========== to ========== [Devtools] Fixed url in Audits This patch fixes areas where NetworkRequest.url conversion was missed because file lacked doctype. R=dgozman,luoe BUG=680054 ==========
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org Link to the patchset: https://codereview.chromium.org/2627073003/#ps80001 (title: "fixed audits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484876448294540,
"parent_rev": "2a7b738269926265f91687afaf177b77814f3b99", "commit_rev":
"5b078604227b8dd8864657954161097abc3091ee"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Fixed url in Audits This patch fixes areas where NetworkRequest.url conversion was missed because file lacked doctype. R=dgozman,luoe BUG=680054 ========== to ========== [Devtools] Fixed url in Audits This patch fixes areas where NetworkRequest.url conversion was missed because file lacked doctype. R=dgozman,luoe BUG=680054 Review-Url: https://codereview.chromium.org/2627073003 Cr-Commit-Position: refs/heads/master@{#444962} Committed: https://chromium.googlesource.com/chromium/src/+/5b078604227b8dd8864657954161... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/5b078604227b8dd8864657954161... |
