Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562193002:
Add filtering by priority in the Network filter area. (Closed)
|
Created:
4 years ago by Oleksii Kadurin 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. |
DescriptionAdd filtering by priority in the Network filter area.
BUG=655203
Committed: https://crrev.com/3c8def3b7ec0e9be5f76c7881fdffd574c72e0c7
Cr-Commit-Position: refs/heads/master@{#440811}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Add filtering by priority in the Network filter area. #Patch Set 3 : Changes made according to reviewer comments #
Total comments: 12
Patch Set 4 : Changes #
Total comments: 17
Patch Set 5 : After reveiw #
Total comments: 3
Patch Set 6 : Changes #
Total comments: 1
Patch Set 7 : Doctype #
Messages
Total messages: 32 (11 generated)
ovkadurin@gmail.com changed reviewers: + allada@chromium.org, chenwilliam@chromium.org
PTAL. It's my first patch. Please run tests for me.
Hi, Thanks for the patch! Going through the process with your first patch is quite painful. We have quite a strict code quality and style but sadly little documentation on it; so it can take quite a while to get a patch through. Try not to be scared away from the comments and suggestions and don't be afraid to ask questions. Overall it looks good, but lets see what it looks like if we have another map of lookup values. Thanks again! https://codereview.chromium.org/2562193002/diff/1/AUTHORS File AUTHORS (left): https://codereview.chromium.org/2562193002/diff/1/AUTHORS#oldcode1 AUTHORS:1: # Names should be added to this file with this pattern: I am not sure what happened here, but I believe this patch may be converting all \n to \r\n or something. Can you please verify/fix this? https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:509: Components.getPriorityByPriorityLabel = function(priorityLabel) { Instead of doing this lets generate an inverse map. example: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:88: if (key === 'priority') { Instead of storing the symbols lets just make sure we pass in the values (since they must be unique anyway). Then use an lookup map to get the symbol where needed. https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:112: static GetSymbolicToNumericPriority() { NetworkRequest is part of the SDK module. Since this code is specific to network module we do not want to put it in SDK since SDK is designed only to represent backend data.
Hi Blaise! Thank you very much for so comprehensive code review. I will check out you comments and then will make needed changes.
Blaise, I added some question below you comments. And updated only AUTHORS file. Other files are not updated yet. Also before update of AUTHORS file I run git pull --rebase origin master. That command brought me a new line in the AUTHORS file. Then after my changes in the file I uploaded it. And now in the patch area the file contains two changes - my one and the one from the master branch. Is ok? Is it how it should be? It's not really clear for me how you are supposed to differ my changes and those came from the master. Thank you for help! https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:509: Components.getPriorityByPriorityLabel = function(priorityLabel) { On 2016/12/12 17:29:57, Blaise wrote: > Instead of doing this lets generate an inverse map. > > example: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... 1. 'inverse map' - do you mean every time the function getPriorityByPriorityLabel is called it should generate inverse map and use the function's key argument as a key of inverse map to return a value? 2. In the example you provided they generate array of values by key/value object. Sorry, but I don't really understand how it might help me. Could you please be more specific here? https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:88: if (key === 'priority') { On 2016/12/12 17:29:57, Blaise wrote: > Instead of storing the symbols lets just make sure we pass in the values (since > they must be unique anyway). Then use an lookup map to get the symbol where > needed. It could be optimized but I don't feel it could be done too much. The thing is that the _values function is called in here https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... and every value here should be transformed label, cause an the line 70 (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) there's a comparison of what a user inputted (he/she knows only about transformed labels like 'lowest'). So conclusion #1. All values returned from the _values function should be transformed. Then at the line 74 (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) they pass array of suggestions that will be shown to user. At this point or later those should be sorted by priority. So conclusion #2. Sorting happens here. But still there's a room for optimization. Sorting should happens after filtering loop. https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:112: static GetSymbolicToNumericPriority() { On 2016/12/12 17:29:57, Blaise wrote: > NetworkRequest is part of the SDK module. Since this code is specific to network > module we do not want to put it in SDK since SDK is designed only to represent > backend data. I see. What do you think would it be a good approach if I make such a static function in the NetworkDataGridNode.js file (https://codereview.chromium.org/2562193002/patch/1/10004) and call it from FilterSuggestionBuilder.js?
>Also before update of AUTHORS file I run git pull --rebase origin master. That >command brought me a new line in the AUTHORS file. Then after my changes in the >file I uploaded it. And now in the patch area the file contains two changes - my >one and the one from the master branch. Is ok? Is it how it should be? It's not >really clear for me how you are supposed to differ my changes and those came >from the master. This is common, don't worry about when this happens, we are used to it on our side. We use rebase or merge ourselves. The only thing we try to do different is upload the patch we just did after the rebase without any other changes then another patch with our changes. This allows us to clearly see what was changed in your patch. But try not to worry much about it. https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:509: Components.getPriorityByPriorityLabel = function(priorityLabel) { On 2016/12/12 21:13:15, Oleksii Kadurin wrote: > On 2016/12/12 17:29:57, Blaise wrote: > > Instead of doing this lets generate an inverse map. > > > > example: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > 1. 'inverse map' - do you mean every time the function > getPriorityByPriorityLabel is called it should generate inverse map and use the > function's key argument as a key of inverse map to return a value? > 2. In the example you provided they generate array of values by key/value > object. Sorry, but I don't really understand how it might help me. Could you > please be more specific here? We already generate a map one time for ENUM => symbol, but I'm suggesting at the same time we generate another UIStringKey => symbol map. This way it'll allow us just lookup what symbol to use from a UIStringKey. https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:88: if (key === 'priority') { On 2016/12/12 21:13:15, Oleksii Kadurin wrote: > On 2016/12/12 17:29:57, Blaise wrote: > > Instead of storing the symbols lets just make sure we pass in the values > (since > > they must be unique anyway). Then use an lookup map to get the symbol where > > needed. > > It could be optimized but I don't feel it could be done too much. The thing is > that the _values function is called in here > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > and every value here should be transformed label, cause an the line 70 > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) > there's a comparison of what a user inputted (he/she knows only about > transformed labels like 'lowest'). > So conclusion #1. All values returned from the _values function should be > transformed. > > Then at the line 74 > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) > they pass array of suggestions that will be shown to user. At this point or > later those should be sorted by priority. > So conclusion #2. Sorting happens here. > > But still there's a room for optimization. Sorting should happens after > filtering loop. > Lets first address the two way map from the other comment. Then we can address this one easier (I think). https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:112: static GetSymbolicToNumericPriority() { On 2016/12/12 21:13:15, Oleksii Kadurin wrote: > On 2016/12/12 17:29:57, Blaise wrote: > > NetworkRequest is part of the SDK module. Since this code is specific to > network > > module we do not want to put it in SDK since SDK is designed only to represent > > backend data. > > I see. What do you think would it be a good approach if I make such a static > function in the NetworkDataGridNode.js file > (https://codereview.chromium.org/2562193002/patch/1/10004) and call it from > FilterSuggestionBuilder.js? Lets just expose: Network.NetworkDataGridNode._symbolicToNumericPriority by removing the _. (in the other file though)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:88: if (key === 'priority') { On 2016/12/12 23:51:08, allada wrote: > On 2016/12/12 21:13:15, Oleksii Kadurin wrote: > > On 2016/12/12 17:29:57, Blaise wrote: > > > Instead of storing the symbols lets just make sure we pass in the values > > (since > > > they must be unique anyway). Then use an lookup map to get the symbol where > > > needed. > > > > It could be optimized but I don't feel it could be done too much. The thing is > > that the _values function is called in here > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > and every value here should be transformed label, cause an the line 70 > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) > > there's a comparison of what a user inputted (he/she knows only about > > transformed labels like 'lowest'). > > So conclusion #1. All values returned from the _values function should be > > transformed. > > > > Then at the line 74 > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...) > > they pass array of suggestions that will be shown to user. At this point or > > later those should be sorted by priority. > > So conclusion #2. Sorting happens here. > > > > But still there's a room for optimization. Sorting should happens after > > filtering loop. > > > > Lets first address the two way map from the other comment. Then we can address > this one easier (I think). Sorry, I still don't get the idea here :( I will just write my vision of the purpose of this change once again and if possible, please point out my mistakes. So this function returns label that should be displayed in the suggestion box. Those labels are unique priority labels collected from all requests in the Network log. So if the log doesn't contain for instance 'VeryLow' priority, then the function shouldn't return that value in the array. Then I request the 'Enum -> numeric priority' map. Then sort unique ENUMS by priority from the very lowest to the highest. Then transform all the Enums in the array to corresponding labels. That array of label will be used for showing labels in the suggestion box. https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:516: .reduce(((map, priority) => map.set(labelMap.get(priority), priority)), new Map()); * I know that such a huge indentation here looks ugly. But it's what clang-format have me to do. * I move the property 'labelMap' out of the 'if' block. But I'm not sure how it works at the end. I've noticed that usually code style differs from what I've seen before. It's ok here to have 'var' declarations inside the condition/loop blocks, even though there's only function scope when it comes to 'var' declarations. Is it because at the end all the JS code is somehow transpiled to something else to have ability to run devtools as a part of the whole browser? * Unfortunately I'm still not sure if what I made in case of 'labelToPriorityMap' is a proper creation of inverse map.
Looking really good. We are almost there! Thanks a lot! https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:492: if (!labelMap) { Can we extract part of this function into "Components.priorityUiLabelMap()" https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:509: Components.priorityByUiLabel = function(priorityLabel) { nit: lets rename to: "uiLabelToPriority" https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:510: var labelToPriorityMap = Components.priorityByUiLabel._uILabelToPriority; Lets restructure this function to something like: if (Components.uiLabelForPriority._uiLabelToPriorityMap) return Components.uiLabelForPriority._uiLabelToPriorityMap.get(priorityLabel); var map = Components.priorityUiLabelMap().inverse(); Components.uiLabelForPriority._uiLabelToPriorityMap = map; return map.get(priorityLabel) || ''; https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:514: labelMap = Components.uiLabelForPriority._priorityToUILabel; This may not be set yet. https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:88: if (key === 'priority') { nit: if (key === Network.NetworkLogView.FilterType.Priority) https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:89: priorityMap = Network.NetworkDataGridNode.symbolicToNumericPriority; If we change in the other file to send over the uiString, we could then work with the uiStrings instead of the enums. eg: var resultSet = new Set(result); result = []; var numericToPriorityMap = Network.NetworkDataGridNode.symbolicToNumericMap().inverse(); var sortedNumericPriorities = numericToPriorityMap.keysArray().sortNumbers(); var sortedPriorities = sortedNumericPriorities.map(value => numericToPriorityMap.get(value)); for (var value of sortedPriorities) { var uiPriority = resultSet.get(value); if (!uiPriority) continue; result.push(uiPriority); } return result; https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:173: Network.NetworkDataGridNode.symbolicToNumericPriority = new Map(); Lets extract part of this into something like: "symbolicToNumericMap()" function. I know you already did this once and put it back, but lets put it in NetworkConditionsSelector. Can you please also put a todo above the function reading: // TODO(allada) This function deserves to be in a network-common of some sort. https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:217: return request.initialPriority() === Components.priorityByUiLabel(value); We should then be able to directly compare (if we do below suggestion) and this will make less calls to Component's function. eg: request.initialPriority() === value. https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:959: this._suggestionBuilder.addItem(Network.NetworkLogView.FilterType.Priority, request.initialPriority()); Lets change this to: this._suggestionBuilder.addItem(Network.NetworkLogView.FilterType.Priority, Components.uiLabelForPriority(request.initialPriority())); https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1471: return Network.NetworkLogView._requestPriorityFilter.bind(null, value); Lets bind value to the NetworkPriority item: return Network.NetworkLogView._requestPriorityFilter.bind(null, Components.uiLabelToPriority(value));
I made some changes. Thanks for review! https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:89: priorityMap = Network.NetworkDataGridNode.symbolicToNumericPriority; >>> var resultSet = new Set(result); What is the point in creating Set here? Double check for uniqueness? >>> Network.NetworkDataGridNode.symbolicToNumericMap().inverse(); inverse function returns MultyMap, every value of which has type of Set. So in order to get a string value by a key it's needed to invoke it like "inversedMap.get(key).next().value". As for me it looks not good. If it's ok please let me know. To avoid such syntax I modified 'inverse' function a little. Not it can return ordinary map. >>> var uiPriority = resultSet.get(value); I couldn't find such an API. Hence I modified the piece of code you provided.
allada@chromium.org changed reviewers: + lushnikov@chromium.org
Patch looks really good, Good job! Just a few things. In the mean time I'm going to add another reviewer (+lushnikov) in for prep for final check after the suggested changes are made. Thanks a lot! https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:492: return map.get(priority) || Common.UIString('Unknown'); Lets just return empty string if no match (no need to wrap empty string in Common.UIString()) https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:505: labelToPriorityMap = Components.priorityUiLabelMap().inverse({regularMap: true}); Lets use: labelToPriorityMap = new Map(); Components.priorityUiLabelMap().forEach((value, key) => labelToPriorityMap.set(value, key)); https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:511: * @return {!Map} nit: Incomplete definition @return {!Map<!Protocol.Network.ResourcePriority, string>} https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:519: map = new Map([ nit: This code should look similar to the code below or code below should look similar to this code. (I personally prefer below style, but up to you :-) ) https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:519: map = new Map([ nit: I am trying to doctype as much as possible, so can we doctype this (or closure will not be smart enough to know what's inside it)? ie: /** @type {!Map<!Protocol.Network.ResourcePriority, string>} */ map = new Map..... https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:532: * @return {!Map} nit: Incomplete definition @return {!Map<!Protocol.Network.ResourcePriority, numeric>} https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:534: Components.symbolicToNumericMap = function() { Sorry, lets rename this: "prioritySymbolToNumericMap" (forgot this would be in Components). https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:537: if (!priorityMap) { nit: We use early returns when possible to reduce nested code. eg: if (priorityMap) return priorityMap; .. rest of code ... https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:538: Components.symbolicToNumericMap._symbolicToNumericPriorityMap = new Map(); nit: I am trying to doctype as much as possible, so can we doctype this (or closure will not be smart enough to know what's inside it)? ie: /** @type {!Map<!Protocol.Network.ResourcePriority, number>} */ Components.symbolicToNumericMap._symbolicToNumericPriorityMap = new Map(); https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js (right): https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:82: var result = this._valueLists[key]; nit: Just to try and help closure catch any future bugs lets doctype this while we are here: var result = /** @type {!Array<string>} */ (this._valueLists[key]); https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:90: var numericToPriorityMap = Components.symbolicToNumericMap().inverse({regularMap: true}); Lets replace this with: /** @type {!Map<number, !Protocol.Network.ResourcePriority} */ var numericToPriorityMap = new Map(); Components.symbolicToNumericMap().forEach((value, key) => numericToPriorityMap.set(value, key)); https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/FilterSuggestionBuilder.js:91: var sortedNumericPriorities = numericToPriorityMap.keysArray().sortNumbers(); Lets make this: var sortedNumericPriorities = numericToPriorityMap.keysArray(); sortedNumericPriorities.sortNumbers(); https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/platform/utilities.js (right): https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:487: * @return {!Array.<number>} I didn't realize sort() sorted in place. Lets follow the way sort() works and revert this. https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:499: return this.sort(numericComparator); and this. https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:1161: var result = (config && config.regularMap) ? new Map() : new Multimap(); I spoke to one of the members of our team and he did not feel we should alter this function. Lets revert this and adhoc it. (see other comment)
Hi! Thank you very much for reviewing! Right now I can't submit changes, because the compiler complains about multiple pieces of code. The root of all the complaints is this line: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... Is initialPriority() really can return null-value? Cause if really can, then I can rewrite the code at every point the compiler complains except this one: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... I need some information what to output if the priority is null. Also I left some comment at previous CL with exact messages that compiler complains.
https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (left): https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:181: var aScore = priorityMap.get(a._request.initialPriority()) || 0; The compiler now complains: WARNING - actual parameter 1 of Map.prototype.get does not match formal parameter found : (Protocol.Network.ResourcePriority|null) required: Protocol.Network.ResourcePriority<string> var aScore = priorityMap.get(aRequest.initialPriority()) || 0; ^^^^^^^^^^^^^^^^^^^^^^^^^^ Also it complains at this line: NetworkDataGridNode.js:435: WARNING - actual parameter 1 of Components.uiLabelForPriority does not match formal parameter found : (Protocol.Network.ResourcePriority|null) required: Protocol.Network.ResourcePriority<string> this._setTextAndTitle(cell, Components.uiLabelForPriority(this._request.initialPriority())); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:959: Network.NetworkLogView.FilterType.Priority, Components.uiLabelForPriority(request.initialPriority())); WARNING - actual parameter 1 of Components.uiLabelForPriority does not match formal parameter found : (Protocol.Network.ResourcePriority|null) required: Protocol.Network.ResourcePriority<string> Network.NetworkLogView.FilterType.Priority, Components.uiLabelForPriority(request.initialPriority())); ^^^^^^^^^^^^^^^^^^^^^^^^^
On 2016/12/20 21:13:01, Oleksii Kadurin wrote: > https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js > (left): > > https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:181: > var aScore = priorityMap.get(a._request.initialPriority()) || 0; > The compiler now complains: > WARNING - actual parameter 1 of Map.prototype.get does not match formal > parameter > found : (Protocol.Network.ResourcePriority|null) > required: Protocol.Network.ResourcePriority<string> > var aScore = priorityMap.get(aRequest.initialPriority()) || 0; > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Also it complains at this line: > NetworkDataGridNode.js:435: WARNING - actual parameter 1 of > Components.uiLabelForPriority does not match formal parameter > found : (Protocol.Network.ResourcePriority|null) > required: Protocol.Network.ResourcePriority<string> > this._setTextAndTitle(cell, > Components.uiLabelForPriority(this._request.initialPriority())); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js > (right): > > https://codereview.chromium.org/2562193002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:959: > Network.NetworkLogView.FilterType.Priority, > Components.uiLabelForPriority(request.initialPriority())); > WARNING - actual parameter 1 of Components.uiLabelForPriority does not match > formal parameter > found : (Protocol.Network.ResourcePriority|null) > required: Protocol.Network.ResourcePriority<string> > Network.NetworkLogView.FilterType.Priority, > Components.uiLabelForPriority(request.initialPriority())); > > ^^^^^^^^^^^^^^^^^^^^^^^^^ Can you upload your code by using something like: git cl upload --bypass-hooks This will allow me to see your jsdocs. I believe these are caused because of something minor in the JSDocs. These errors are actually a good thing. They mean that closure was able to find stuff it previously was not able to find.
I uploaded last changes.
Try these :-) https://codereview.chromium.org/2562193002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2562193002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:240: var aScore = priorityMap.get(aRequest.initialPriority()) || 0; Try: var aPriority = aRequest.initialPriority(); var aScore = aPriority ? priorityMap.get(aPriority) : 0; var bPriority = aRequest.initialPriority(); var bScore = bPriority ? priorityMap.get(bPriority) : 0; https://codereview.chromium.org/2562193002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:435: this._setTextAndTitle(cell, Components.uiLabelForPriority(this._request.initialPriority())); Change this to: var priority = this._request.initialPriority(); this._setTextAndTitle(cell, priority ? Components.uiLabelForPriority(priority) : ''); https://codereview.chromium.org/2562193002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2562193002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:981: this._suggestionBuilder.addItem( Change this to: var priority = request.initialPriority(); if (priority) { this._suggestionBuilder.addItem(Network.NetworkLogView.FilterType.Priority, Components.uiLabelForPriority(priority)); }
allada@chromium.org changed reviewers: + dgozman@chromium.org - lushnikov@chromium.org
Awesome! lgtm. I am going to add +dgozman to this for a final review. I am also going to do a bot dryrun to check to make sure it did not break any tests. Thanks a lot! https://codereview.chromium.org/2562193002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js (right): https://codereview.chromium.org/2562193002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/NetworkConditionsSelector.js:500: var labelToPriorityMap = Components.uiLabelToPriority._uiLabelToPriorityMap; nit: Lets doctype this (above this line): /** @type {!Map<string, !Protocol.Network.ResourcePriority>} */
The CQ bit was checked by allada@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...
Thank you for review!
lgtm
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2562193002/#ps140001 (title: "Doctype")
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": 140001, "attempt_start_ts": 1482875599636990,
"parent_rev": "93496b6d6056327cc6957511aaae4741fc852737", "commit_rev":
"6a38802fa1ed42c09aeea0648575b4e2e16dd8ce"}
Message was sent while issue was closed.
Description was changed from ========== Add filtering by priority in the Network filter area. BUG=655203 ========== to ========== Add filtering by priority in the Network filter area. BUG=655203 Review-Url: https://codereview.chromium.org/2562193002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add filtering by priority in the Network filter area. BUG=655203 Review-Url: https://codereview.chromium.org/2562193002 ========== to ========== Add filtering by priority in the Network filter area. BUG=655203 Committed: https://crrev.com/3c8def3b7ec0e9be5f76c7881fdffd574c72e0c7 Cr-Commit-Position: refs/heads/master@{#440811} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3c8def3b7ec0e9be5f76c7881fdffd574c72e0c7 Cr-Commit-Position: refs/heads/master@{#440811} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
